Closed
Bug 1386514
Opened 7 years ago
Closed 7 years ago
Support RWD for about:preferences to match Photon visual spec
Categories
(Firefox :: Settings UI, enhancement, P1)
Firefox
Settings UI
Tracking
()
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: rickychien, Assigned: evanxd)
References
(Blocks 1 open bug)
Details
(Whiteboard: [photon-preference])
Attachments
(3 files)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → evan
Status: NEW → ASSIGNED
Priority: P2 → P1
Updated•7 years ago
|
Whiteboard: [photon-preference][triage] → [photon-preference]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8895273 -
Flags: review?(mconley)
Assignee | ||
Comment 2•7 years ago
|
||
Hi Mike, Could you help review the patch? And this is the RWD spec[1]. [1]: https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/245152762
Assignee | ||
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review171640 ::: browser/themes/shared/incontentprefs/preferences.inc.css:17 (Diff revision 1) > } > > .pane-container { > display: block; > - max-width: 800px; > + max-width: 664px; > + min-width: 664px; Hi Mike, I'm not sure why need to use `max-width: 664px;` and `min-width: 664px;` to keep the width fixed as 664px. Using `width: 664px;` (without `max-width` and `min-width`) doesn't work for setting the width. Do you know why?
Comment 4•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review171640 > Hi Mike, > > I'm not sure why need to use `max-width: 664px;` and `min-width: 664px;` to keep the width fixed as 664px. Using `width: 664px;` (without `max-width` and `min-width`) doesn't work for setting the width. > > Do you know why? If I had to guess, I'd say "XUL box model". jaws might have a more definitive answer - been a while since I've had to deal with XUL sizing, I'm afraid.
Assignee | ||
Comment 5•7 years ago
|
||
Hi Jared, Do you think it make sense we use the below css rules in the patch[1] to keep the `.pane-container` element have fixed width as 664? ``` .pane-container { display: block; max-width: 664px; min-width: 664px; } ``` I tried to use the below css rules to do so but it doesn't work. Do you have any other idea to let the `.pane-container` element have a fixed width in this case? ``` .pane-container { display: block; width: 664px; } ``` Thank you. [1]: https://reviewboard.mozilla.org/r/166444/diff/1#index_header
Flags: needinfo?(jaws)
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review172176 ::: browser/themes/shared/incontentprefs/preferences.inc.css:11 (Diff revision 1) > @namespace html "http://www.w3.org/1999/xhtml"; > > .main-content { > padding-top: 0; > background-color: #fafafc; > + padding-inline-start: 28px !important; Please make this change in common.inc.css.
Comment 8•7 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #4) > Comment on attachment 8895273 [details] > Bug 1386514 - Implement RWD to match Photon visual spec. > > https://reviewboard.mozilla.org/r/166444/#review171640 > > > Hi Mike, > > > > I'm not sure why need to use `max-width: 664px;` and `min-width: 664px;` to keep the width fixed as 664px. Using `width: 664px;` (without `max-width` and `min-width`) doesn't work for setting the width. > > > > Do you know why? > > If I had to guess, I'd say "XUL box model". jaws might have a more > definitive answer - been a while since I've had to deal with XUL sizing, I'm > afraid. This node doesn't use XUL layout, because display: block is used here. Mixing XUL and non-XUL layout is generally fragile. Why was display: block used here?
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review172582 ::: browser/themes/shared/incontentprefs/preferences.inc.css:11 (Diff revision 1) > @namespace html "http://www.w3.org/1999/xhtml"; > > .main-content { > padding-top: 0; > background-color: #fafafc; > + padding-inline-start: 28px !important; Sure, let's do it. Thank you for pointing out.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #8) > (In reply to Mike Conley (:mconley) from comment #4) > This node doesn't use XUL layout, because display: block is used here. > Mixing XUL and non-XUL layout is generally fragile. Why was display: block > used here? Hi Dão, According to my understand, `display: block` is a workaround[1] to keep the container always float on the `top: 0` which added at Bug 1377009. [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1377009#c11
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review171640 > If I had to guess, I'd say "XUL box model". jaws might have a more definitive answer - been a while since I've had to deal with XUL sizing, I'm afraid. Fixed it by adding `align=start` into it's parent node since the default vaule of `align` is `stretch` which means the child elements are stretched to fit the size of the parent elements.
Assignee | ||
Comment 15•7 years ago
|
||
Hi Mike, Fixed the `max-width` and `min-width` issue. Could you help review it again? Thank you.
Flags: needinfo?(jaws)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8895273 -
Flags: review?(mconley) → review?(jaws)
Assignee | ||
Comment 18•7 years ago
|
||
Hi Jared, Since Mike is on PTO, could you help review the patch? Thank you.
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review173484 ::: browser/themes/shared/incontentprefs/preferences.inc.css:15 (Diff revision 6) > - max-width: 800px; > + width: 664px; > + min-width: 664px; I have seen the spec but I don't understand why we need a min-width here? Much of the content will already resize correctly to narrower widths. This seems to make the experience for people on small resolutions worse. For example, on Nightly 46.0a1 (2016-01-01), the full window of the preferences resizes to an innerWidth of 517px before horizontal scrollbars appear. This patch sets a min-width of 664px just on the .pane-container, which makes the min-width of the full window larger than 720px. ::: browser/themes/shared/incontentprefs/preferences.inc.css:161 (Diff revision 6) > + #categories { > + width: 118px; > + } > + .category, > + .category:hover, > + .category[selected] { > + padding-inline-start: 47px; /* make category icons align center */ In small windows, why do we use so much space to pad the category icons? This takes away useful space from .pane-container.
Attachment #8895273 -
Flags: review?(jaws) → review-
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review173486 ::: commit-message-9f812:1 (Diff revision 6) > +Bug 1386514 - Implement RWD to match Photon visual spec. Please explain more of what the patch is doing. "RWD" as in "Responsive Web Design" is not detailed enough.
Reporter | ||
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review173786 ::: browser/themes/shared/incontentprefs/preferences.inc.css:161 (Diff revision 6) > + #categories { > + width: 118px; > + } > + .category, > + .category:hover, > + .category[selected] { > + padding-inline-start: 47px; /* make category icons align center */ Can we move this changes to common.inc.css?
Reporter | ||
Comment 22•7 years ago
|
||
Bug 1377174 is moving category changes to common.inc.css. For instance, `width: 240px` will be added in *|*.category within common.inc.css. Other in-content pages would be broken if RWD style only apply in preferences.inc.css.
Assignee | ||
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review173872 ::: browser/themes/shared/incontentprefs/preferences.inc.css:15 (Diff revision 6) > - max-width: 800px; > + width: 664px; > + min-width: 664px; Hi Jared, According to my understand of the spec, it require a fixed as 664px width for the right side's content. But you make a good point here. And this is a design issue. Let's hear feedback from designer, Helen. But she is on PTO today. Let's discuss this when she is back. ::: browser/themes/shared/incontentprefs/preferences.inc.css:161 (Diff revision 6) > + #categories { > + width: 118px; > + } > + .category, > + .category:hover, > + .category[selected] { > + padding-inline-start: 47px; /* make category icons align center */ To Jared, It is also a design issue. Let's wait for Helen's feedback. To Ricky, Sure, let's do it.
Assignee | ||
Comment 24•7 years ago
|
||
Hi Helen, What do you think of Comment 19? And let's discuss it in person tomorrow when you have time. Thank you very much.
Flags: needinfo?(hhuang)
Assignee | ||
Comment 25•7 years ago
|
||
Discussed with Helen in person this morning, she is considering the details of the visual design change for min-width (we might use `min-width: 530px` here) and category list issues in Comment 19 and and will reply it later. And let's see where we want to go and get feedback form Jared, and let's update the patch for it.
Comment 26•7 years ago
|
||
I see Jared's point that set 664px as mini-width will work badly on the small resolution. Per discussion with Evan, we use Privacy & Security page as an example. When the minimum width of the content area is 530px, the characters of the shortest sentence is around 47 per line, and it could still provide a good reading experience (ideal range is 40-60), so maybe it's okay to set 530px as min-width. Jared, Is it okay for you?
Flags: needinfo?(hhuang) → needinfo?(jaws)
Comment 27•7 years ago
|
||
Yes, increasing to 530px is an improvement. Do you think we would be able to reduce the amount of space between the category list and the pane content? In your screenshot, there is somewhere around 30px more space to the right of the category icons than to the left of the category icons. Ideally we should balance this, right? I have attached a screenshot showing my measurements.
Flags: needinfo?(jaws)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•7 years ago
|
||
Rebased and updated patch for Comment 26. The last thing we need to make a decision and do is fixing the Comment 27. Will discuss it with Helen in person later.
Comment hidden (mozreview-request) |
Comment 31•7 years ago
|
||
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try searching for changes remote: abort: abandoned transaction found! remote: (run 'hg recover' to clean up transaction) abort: stream ended unexpectedly (got 0 bytes, expected 4)
Assignee | ||
Comment 32•7 years ago
|
||
Hi Jared, Discussed with Helen in person, we think we could remove the 28px padding when on the small window (>= 960px) mode. She will update the spec when she has time. Thank you, Helen. And I've updated the patch for our conclusions. Could you please review it again? Thank you.
Flags: needinfo?(hhuang)
Comment 33•7 years ago
|
||
I've modified the spec accordingly, please see the updated spec https://mozilla.invisionapp.com/share/X8BGCX9PD#/245152761_RWD_-_W840. Thanks!
Flags: needinfo?(hhuang)
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review174986 This looks mostly OK, but the lack of padding between .pane-content and the edge of the window when the window is narrow doesn't look polished. See https://www.screencast.com/t/sHlkH0wIs for a screenshot. If we can add 10px of padding there, and if Helen is OK with it, then I think this will be good to land. The spec doesn't show how the window should look when we have a narrow window and we are not cutting off the content, so it is not obvious what was desired in this state.
Attachment #8895273 -
Flags: review?(jaws) → review-
Assignee | ||
Comment 35•7 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #34) > This looks mostly OK, but the lack of padding between .pane-content and the > edge of the window when the window is narrow doesn't look polished. See > https://www.screencast.com/t/sHlkH0wIs for a screenshot. > > If we can add 10px of padding there, and if Helen is OK with it, then I > think this will be good to land. The spec doesn't show how the window should > look when we have a narrow window and we are not cutting off the content, so > it is not obvious what was desired in this state. Sure, let's get feedback from Helen.
Flags: needinfo?(hhuang)
Comment 36•7 years ago
|
||
That makes sense to me, I will update the spec. Thanks for your suggestion, Jared.
Flags: needinfo?(hhuang)
Comment 37•7 years ago
|
||
mozreview-review |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review175172 ::: toolkit/themes/shared/in-content/common.inc.css:696 (Diff revision 8) > + .category, > + .category:hover, > + .category[selected] { Why do we need extra selectors for `.category:hover` and `.category[selected]`? Shouldn't `.category` be good enough?
Assignee | ||
Comment 38•7 years ago
|
||
mozreview-review |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review175192 ::: toolkit/themes/shared/in-content/common.inc.css:696 (Diff revision 8) > + .category, > + .category:hover, > + .category[selected] { You're right. Let's update it.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 42•7 years ago
|
||
Hi Jared, I've updated patch for review comments and added the 10px margin. Could you review it again? Thank you.
Comment 43•7 years ago
|
||
mozreview-review |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review175462 r=me if you move the margin-inline-end to .pane-container ::: toolkit/themes/shared/in-content/common.inc.css:710 (Diff revisions 8 - 11) > } > > .main-content { > padding-left: 0; > padding-right: 0; > + margin-inline-end: 10px; When I make the window narrower this margin ends up on the right side of the scrollbar which is very odd. If I move this margin-inline-end to .pane-container then the problem is fixed and all looks good.
Attachment #8895273 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 44•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895273 [details] Bug 1386514 - Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. https://reviewboard.mozilla.org/r/166444/#review175462 > When I make the window narrower this margin ends up on the right side of the scrollbar which is very odd. > > If I move this margin-inline-end to .pane-container then the problem is fixed and all looks good. Sure, let's do it.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 46•7 years ago
|
||
Updated the patch for the review comment. Let's land it once the try[1] is good. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d76a8f2efbda
Comment 47•7 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/67ad8661e46d Implement Responsive Web Design to make `.pane-container` element's width as 530px to 664px and make category and question mark icons align center when window's width is smaller than 830px. r=jaws
Comment 51•7 years ago
|
||
Build ID: 20170905220108 User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 Verified as fixed on Firefox Nightly 57.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•