Update about:preferences separator according to Photon visual spec

VERIFIED FIXED in Firefox 57

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: rickychien, Assigned: evanxd)

Tracking

(Blocks 1 bug)

55 Branch
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-preference])

Attachments

(1 attachment)

According latest Photon about:preferences visual spec [1], we need to do following changes:

* Show separators for each preferences sub-panel (#general, #search, #privacy, #sync) by default when sub-panel is selected.
* Show necessary separators when user performs a search.

[1] https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244683138
Flags: qe-verify+
Priority: P1 → P2
Whiteboard: [photon-preference][triage] → [photon-preference]
Assignee

Updated

2 years ago
Assignee: nobody → evan
Priority: P2 → P1
Assignee

Updated

2 years ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8893703 - Flags: review?(jaws)
Assignee

Comment 3

2 years ago
Hi Jared,

Could you help review the patch?
We would like to add separators for each level 1 header in Photon refresh work.

Thank you.

Comment 4

2 years ago
mozreview-review
Comment on attachment 8893703 [details]
Bug 1386160 - Add separators for each level 1 header to match the Photon visual spec.

https://reviewboard.mozilla.org/r/164820/#review170342

::: browser/components/preferences/in-content-new/privacy.xul:651
(Diff revision 2)
>    </hbox>
>  #endif
>  </groupbox>
>  #endif
>  
> +<separator class="groove" data-category="panePrivacy" hidden="true"/>

We shouldn't need extra elements just to draw this border.

::: browser/themes/shared/incontentprefs/preferences.inc.css:83
(Diff revision 2)
>  /**
>   * The first subcategory title for each category should not have margin-top.
>   */
>  #generalCategory,
>  #searchCategory,
>  #browserPrivacyCategory,
>  #firefoxAccountCategory {
>    margin-top: 0;
>  }
>  
>  .header-name {
>    font-size: 2rem;
>  }
>  
>  .subcategory {
>    margin-top: 48px;
>  }

Delete the section at the top of this that sets margin-top to 0.

Then change the .subcategory rule to what follows:

.subcategory:not([hidden]) ~ .subcategory {
  margin-top: 32px;
  padding-top: 15px;
  border-top: 1px solid grey;
}

This sets the margin-top to 32px for all subcategories that aren't the first visible one, and sets the appropriate padding between the text and the border, while also subtracting the border from the padding as the spec requires.
Attachment #8893703 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8893703 [details]
Bug 1386160 - Add separators for each level 1 header to match the Photon visual spec.

https://reviewboard.mozilla.org/r/164820/#review170342

> Delete the section at the top of this that sets margin-top to 0.
> 
> Then change the .subcategory rule to what follows:
> 
> .subcategory:not([hidden]) ~ .subcategory {
>   margin-top: 32px;
>   padding-top: 15px;
>   border-top: 1px solid grey;
> }
> 
> This sets the margin-top to 32px for all subcategories that aren't the first visible one, and sets the appropriate padding between the text and the border, while also subtracting the border from the padding as the spec requires.

Sure, let's do it.
Assignee

Comment 9

2 years ago
Hi Jared,

Thank you for the tips. I've updated the patch for the review comments.
Could you please review it again?

Thank you.

Comment 10

2 years ago
mozreview-review
Comment on attachment 8893703 [details]
Bug 1386160 - Add separators for each level 1 header to match the Photon visual spec.

https://reviewboard.mozilla.org/r/164820/#review171970

::: browser/themes/shared/incontentprefs/preferences.inc.css:84
(Diff revision 5)
>   * The first subcategory title for each category should not have margin-top.
>   */
> -#generalCategory,
> -#searchCategory,
> -#browserPrivacyCategory,
> -#firefoxAccountCategory {
> +
> +.subcategory:not([hidden]) ~ .subcategory {
> +  margin-top: 32px;
> +  padding-top: 16px;

Should this be padding-top: 15px to account for the 1px border?

::: browser/themes/shared/incontentprefs/preferences.inc.css:92
(Diff revision 5)
>  .subcategory {
>    margin-top: 48px;
>  }

Can this rule get deleted now? I don't think it does anything now that the new rule has higher precedence (score of 0030 vs 0010).
Attachment #8893703 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)
Assignee

Comment 12

2 years ago
mozreview-review
Comment on attachment 8893703 [details]
Bug 1386160 - Add separators for each level 1 header to match the Photon visual spec.

https://reviewboard.mozilla.org/r/164820/#review172008

::: browser/themes/shared/incontentprefs/preferences.inc.css:84
(Diff revision 5)
>   * The first subcategory title for each category should not have margin-top.
>   */
> -#generalCategory,
> -#searchCategory,
> -#browserPrivacyCategory,
> -#firefoxAccountCategory {
> +
> +.subcategory:not([hidden]) ~ .subcategory {
> +  margin-top: 32px;
> +  padding-top: 16px;

Yes, you're right. Let's do it.

::: browser/themes/shared/incontentprefs/preferences.inc.css:92
(Diff revision 5)
>  .subcategory {
>    margin-top: 48px;
>  }

Yes, let's remove it. And we don't need to update the rule at Bug 1377174's patch[1].

[1]: https://reviewboard.mozilla.org/r/163258/diff/16#index_header
Assignee

Comment 13

2 years ago
Hi Jared,

I've updated the patch for your comments. Could you please review it again? Thank you.

Comment 14

2 years ago
mozreview-review
Comment on attachment 8893703 [details]
Bug 1386160 - Add separators for each level 1 header to match the Photon visual spec.

https://reviewboard.mozilla.org/r/164820/#review172254
Attachment #8893703 - Flags: review?(jaws) → review+
Assignee

Comment 15

2 years ago
Thank you for reviewing, Jared.

And the try is good, let's land the patch.
Keywords: checkin-needed

Comment 16

2 years ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea0c21ef7e3c
Add separators for each level 1 header to match the Photon visual spec. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ea0c21ef7e3c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

Comment 18

2 years ago
I can see the feature "separators for each preferences sub-panel" implemented in latest Nightly 57.0a1 on Windows 10, 64-bit  

Build ID :   20170814100258
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170809]

Comment 19

2 years ago
Build ID: 20170820100343
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.