Differential D303488

Bug 2023763 - Nova styles for application menu r=mstriemer
ClosedPublic
Actions

Authored by nsharpley on Fri, May 29, 8:02 PM.

Details

Reviewers

Group Reviewers

Restricted Project

desktop-theme-reviewers

Commits

rFIREFOXAUTOLAND16e8802bf561: Bug 2023763 - Nova styles for application menu r=mstriemer,fluent-reviewers…
rFIREFOXAUTOLANDc78f6b529878: Bug 2023763 - Nova styles for application menu r=mstriemer,fluent-reviewers…

Bugzilla Bug ID

2023763

Summary

switched to moz-button for sign in
the "zap" gradient under the account menu item is different
new colored box with new styling for update message and the "switching to a new device" menu item under "Help"

You can see these changes by visiting the application menu, and navigating to Help for the last point

Diff Detail

Repository: rFIREFOXAUTOLAND firefox-autoland

Event Timeline

nsharpley planned changes to this revision.Fri, May 29, 8:02 PM

nsharpley created this revision.

Herald added a project: secure-revision. · View Herald TranscriptFri, May 29, 8:02 PM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".Fri, May 29, 8:02 PM

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

Harbormaster failed remote builds in B977944: Diff 1287126!Fri, May 29, 8:37 PM

nsharpley planned changes to this revision.Thu, Jun 4, 8:32 PM

nsharpley updated this revision to Diff 1292487.

reviewbot added a subscriber: reviewbot.Thu, Jun 4, 8:54 PM

Comment Actions

Code analysis found 4 defects in diff 1292487:

1 defect found by eslint (Mozlint)
1 defect found by fluent-lint (Mozlint)
2 defects found by stylelint (Mozlint)

IMPORTANT: Found 4 defects (error level) that must be fixed before landing.

You can run this analysis locally with:

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 1292487.

Harbormaster failed remote builds in B981781: Diff 1292487!Thu, Jun 4, 8:54 PM

nsharpley planned changes to this revision.Mon, Jun 8, 2:47 PM

nsharpley updated this revision to Diff 1293871.

reviewbot added a comment.Mon, Jun 8, 3:08 PM

Comment Actions

Code analysis found 6 defects in diff 1293871:

1 defect found by stylelint (Mozlint)
2 defects found by fluent-lint (Mozlint)
3 defects found by eslint (Mozlint)

IMPORTANT: Found 6 defects (error level) that must be fixed before landing.

You can run this analysis locally with:

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 1293871.

Harbormaster failed remote builds in B982827: Diff 1293871!Mon, Jun 8, 3:08 PM

nsharpley planned changes to this revision.Tue, Jun 9, 2:17 PM

nsharpley updated this revision to Diff 1295278.

nsharpley requested review of this revision.Tue, Jun 9, 4:01 PM

nsharpley updated this revision to Diff 1295398.

nsharpley retitled this revision from WIP: Bug 2023763 - Nova styles for application menu to Bug 2023763 - Nova styles for application menu r=mstriemer.

nsharpley added a reviewer: mstriemer.

Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Jun 9, 4:01 PM

Herald added a reviewer: desktop-theme-reviewers. · View Herald Transcript

Herald added a reviewer: reusable-components-reviewers-rotation. · View Herald Transcript

Herald added a reviewer: desktop-theme-reviewers. · View Herald Transcript

Herald added a reviewer: home-newtab-reviewers-rotation. · View Herald Transcript

Herald added a subscriber: flod. · View Herald Transcript

phab-bot added a reviewer: ini.Tue, Jun 9, 4:03 PM

phab-bot removed reviewers: reusable-components-reviewers-rotation, home-newtab-reviewers-rotation.

phab-bot added subscribers: reusable-components-reviewers-rotation, home-newtab-reviewers-rotation.

nsharpley added a comment.Tue, Jun 9, 4:04 PM

Comment Actions

I've named some elements that have a Nova variant with "nova" though I am wondering if that's a good idea since it would require a rename when we eventually default to Nova and remove old styling... Thoughts?

bolsson requested changes to this revision.Tue, Jun 9, 5:25 PM

bolsson added a subscriber: bolsson.

bolsson added inline comments.

browser/locales/en-US/browser/appmenu.ftl
22–23	We should have a comment for localizers indicating what a "Fresh Firefox" is referring to (e.g. a "Fresh Firefox refers to the new updated UI")

This revision now requires changes to proceed.Tue, Jun 9, 5:25 PM

nsharpley marked an inline comment as done.Tue, Jun 9, 7:23 PM

nsharpley requested review of this revision.Tue, Jun 9, 7:23 PM

nsharpley updated this revision to Diff 1295604.

nsharpley edited the summary of this revision. (Show Details)

jules accepted this revision.Tue, Jun 9, 7:34 PM

jules resigned from this revision.Tue, Jun 9, 7:39 PM

Comment Actions

Sorry I approved it accidentally thinking it was another patch I had open - will take a closer look

mstriemer requested changes to this revision.Tue, Jun 9, 8:17 PM

Comment Actions

Seems pretty close but a found a few issues

browser/base/content/appmenu-viewcache.inc.xhtml
33–34	s emit events but XUL and elements emitted events This button doesn't work for me anymore I also happened to be signed in when I tested this and it looks a little odd 👁 image.png (1×848 px, 215 KB)
browser/themes/shared/customizableui/panelUI-shared.css
67–69	This feels unfortunate. We should probably set up correctly instead There is a and it could get this added to it: "separator": { "color": { "value": { "default": "color-mix(in srgb, currentColor 25%, transparent)", "prefersContrast": "currentColor" } } }, And then we can delete the current definitions: https://searchfox.org/firefox-main/search?q=--panel-separator-color&path=&case=false&regexp=false Running buildtokens should get the Figma value for Nova
751–753	I think the linter was pointing out a real error
toolkit/content/widgets/moz-promo/moz-promo.css
67	We refer to this as an image for the src and size, so I think we should be consistent
toolkit/themes/shared/design-system/src/tokens/components/icon.tokens.json
12–13	nitpick: whitespace

This revision now requires changes to proceed.Tue, Jun 9, 8:17 PM

nsharpley requested review of this revision.Wed, Jun 10, 2:32 PM

nsharpley updated this revision to Diff 1296334.

nsharpley marked 4 inline comments as done.

nsharpley updated this revision to Diff 1296338.Wed, Jun 10, 2:34 PM

nsharpley marked an inline comment as done.

nsharpley updated this revision to Diff 1296493.Wed, Jun 10, 3:41 PM

ini accepted this revision.Wed, Jun 10, 8:02 PM

Comment Actions

lgtm for newtab changes!

bolsson accepted this revision.Wed, Jun 10, 8:50 PM

mstriemer accepted this revision.Fri, Jun 12, 6:11 PM

mstriemer added a project: testing-approved.

Comment Actions

Looks great, thanks!

browser/themes/shared/customizableui/panelUI-shared.css
586	I think this needs the margin not the padding from the menuitems
toolkit/themes/shared/design-system/src/figma-import.mjs
27 ↗	(On Diff #1296493)	All of the widgets are being imported now rather than the specifically listed components. If you delete this and run you should be able to cleanly rebase onto main. I think another buildtokens at that point shouldn't have any changes