Closed Bug 1388997 Opened 6 years ago Closed 6 years ago

Add hover effects for menu items on preference page.

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: evanxd, Assigned: evanxd)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(1 file)

Bug 1388997 - Add hover effects for menu list to match Photon visual spec.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
Add hover effects for menu items on preference page.
Flags: qe-verify+
Haven't had a visual spec for it yet.

Hi Helen,

Could you provide the spec here once we have it?

Thank you.
Flags: needinfo?(hhuang)
Whiteboard: [photon-preference][triage] → [photon-preference]
The visual spec is ready, link here https://mozilla.invisionapp.com/share/X8BGCX9PD#/244683005_Navigation
Thanks!
Flags: needinfo?(hhuang)
Attachment #8896188 - Flags: review?(jaws)
Hi Jared,

Could you help review the patch?

Thank you.
Did you verify this patch on about:addons too? There is currently no hover state for unselected items there too, and strangely, the font color changes slightly for the selected item.
Flags: needinfo?(evan)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #5)
> the font color changes slightly for the selected item.

... the font color changes slightly for the selected item when hover over.
Hi Tim,

I had verified it previously, about:addons page has hover state for unselected items after applying the patch. But we should fix the color changes slightly issue here. Let me fix it in this bug.
Flags: needinfo?(evan)
Comment on attachment 8896188 [details]
Bug 1388997 - Add hover effects for menu list to match Photon visual spec.

https://reviewboard.mozilla.org/r/167480/#review173504

On about:addons, there was an icon that was hidden but when hovering over it I could see that there was a category due to this. Also, the background highlight doesn't appear symmetrical on about:addons.

::: toolkit/themes/shared/in-content/common.inc.css:650
(Diff revision 2)
>    border-inline-end-width: 0;
> -  padding-inline-start: 40px;
> +  padding-inline-start: 10px;
>    padding-inline-end: 21px;
>    min-height: 40px;
>    transition: background-color 150ms;
> +  margin-left: 30px;

This won't work for RTL.

::: toolkit/themes/shared/in-content/common.inc.css:654
(Diff revision 2)
>    transition: background-color 150ms;
> +  margin-left: 30px;
> +}
> +
> +*|*.category:hover {
> +  background-color: rgba(12, 12, 13, 0.1);

nit, no spaces after commas in the color functions.
Attachment #8896188 - Flags: review?(jaws) → review-
Comment on attachment 8896188 [details]
Bug 1388997 - Add hover effects for menu list to match Photon visual spec.

https://reviewboard.mozilla.org/r/167480/#review173846

::: toolkit/themes/shared/in-content/common.inc.css:650
(Diff revision 2)
>    border-inline-end-width: 0;
> -  padding-inline-start: 40px;
> +  padding-inline-start: 10px;
>    padding-inline-end: 21px;
>    min-height: 40px;
>    transition: background-color 150ms;
> +  margin-left: 30px;

You're right. Let's use `margin-inline-start` to fix it.

::: toolkit/themes/shared/in-content/common.inc.css:654
(Diff revision 2)
>    transition: background-color 150ms;
> +  margin-left: 30px;
> +}
> +
> +*|*.category:hover {
> +  background-color: rgba(12, 12, 13, 0.1);

Sure, let's update it.
Hi Jared,

I updated the patch for the review comments. Could you please review it again?

Thank you.
Comment on attachment 8896188 [details]
Bug 1388997 - Add hover effects for menu list to match Photon visual spec.

https://reviewboard.mozilla.org/r/167480/#review174132

Looks good, but please apply this styling to the .help-button
Attachment #8896188 - Flags: review?(jaws) → review-
Hi Jared,

I'm not sure why we need to apply this styling to the `.help-button` since its hover state is changing the opacity value. Check the `.help-button` spec in [1].

If you also agree we don't need to apply this styling to the `.help-button`, let's r+ this patch and land it. What do you think?

And thank you for the review.

[1]: https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/244683005
Flags: needinfo?(jaws)
Comment on attachment 8896188 [details]
Bug 1388997 - Add hover effects for menu list to match Photon visual spec.

https://reviewboard.mozilla.org/r/167480/#review174364

::: browser/themes/shared/incontentprefs/preferences.inc.css:153
(Diff revision 3)
> +  .category {
> +    padding-inline-end: 10px;
> +    margin-inline-end: 11px;
> +  }

Please rebase bug 1377174. This media query changes have moved to common.inc.css.

::: toolkit/themes/shared/in-content/common.inc.css:654
(Diff revision 3)
>    min-height: 40px;
>    transition: background-color 150ms;
>  }
>  
> +*|*.category:hover {
> +  background-color: rgba(12,12,13,0.1);

Can we add a css variable for `rgba(12, 12, 13, 0.1)`?
Rebased and fixed the conflict caused by Bug 1377174.

Hi Jared,

Could you review it again? And what do you think of Comment 14?

Thank you.
(In reply to Evan Tseng [:evanxd] from comment #14)
> Hi Jared,
> 
> I'm not sure why we need to apply this styling to the `.help-button` since
> its hover state is changing the opacity value. Check the `.help-button` spec
> in [1].
> 
> If you also agree we don't need to apply this styling to the `.help-button`,
> let's r+ this patch and land it. What do you think?

The change in fill opacity from 80% to 90% is not enough to make a noticeable change. Helen, could we apply the same hover styling to the help button as we are going to apply to the category icon?
Flags: needinfo?(jaws) → needinfo?(hhuang)
Comment on attachment 8896188 [details]
Bug 1388997 - Add hover effects for menu list to match Photon visual spec.

https://reviewboard.mozilla.org/r/167480/#review174930

r=me if you change the margin-inline-end to 40px.

Also, I would still like to wait to hear Helen's response about the Help button.

::: toolkit/themes/shared/in-content/common.inc.css:698
(Diff revision 4)
> +  .category {
> +    margin-inline-end: 37px;
> +  }

Where did you get 37px from?

118px wide for #categories
34px margin-inline-start for .category
10px padding-inline-start for .category
24px icon for .category
10px padding-inline-end for .category

This leaves 40px for margin-inline-end for .category

Using 40px looks correct to me.
Attachment #8896188 - Flags: review?(jaws) → review+
Comment on attachment 8896188 [details]
Bug 1388997 - Add hover effects for menu list to match Photon visual spec.

https://reviewboard.mozilla.org/r/167480/#review175156

::: toolkit/themes/shared/in-content/common.inc.css:698
(Diff revision 4)
> +  .category {
> +    margin-inline-end: 37px;
> +  }

Hi Jared,

You're right. We should use 40px here.
I've updated the hover state for Firefox support, please find the spec here https://mozilla.invisionapp.com/share/X8BGCX9PD#/244683005_Navigation
Flags: needinfo?(hhuang)
Updated the patch for the updated spec. Let's land it.
Keywords: checkin-needed
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e07f7098f5c8
Add hover effects for menu list to match Photon visual spec. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e07f7098f5c8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Build ID: 20170822100529
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
Depends on: 1397973
You need to log in before you can comment on or make changes to this bug.