Closed Bug 1386160 Opened 7 years ago Closed 7 years ago

Update about:preferences separator according to Photon visual spec

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Version:
55 Branch
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: rickychien, Assigned: evanxd)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(1 file)

Bug 1386160 - Add separators for each level 1 header to match the Photon visual spec.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
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: nobody → evan
Priority: P2 → P1
Status: NEW → ASSIGNED
Attachment #8893703 - Flags: review?(jaws)
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 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 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.
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 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 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
Hi Jared,

I've updated the patch for your comments. Could you please review it again? Thank you.
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+
Thank you for reviewing, Jared.

And the try is good, let's land the patch.
Keywords: checkin-needed
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: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
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]
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
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: