contentful / forma-36

A design system by Contentful
https://f36.contentful.com
MIT License
331 stars 81 forks source link

[DO NOT MERGE] feat: navigation light mode #2730

Closed Lelith closed 2 months ago

Lelith commented 4 months ago

Purpose of PR

Change the color schema of the Navbar component from dark to light. This is not a new variation but a complete switch of style. The overall structure and layout of the component is unchanged. But some of the classNames are changed in order to be more meaningful.

changeset-bot[bot] commented 4 months ago

⚠️ No Changeset found

Latest commit: a72e87ac6ac62b0902525c3514ba21b3d0351870

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel[bot] commented 4 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
forma-36 ✅ Ready (Inspect) Visit Preview Jul 17, 2024 2:30pm
github-actions[bot] commented 4 months ago

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
CommonJS 136.05 KB (+0.05% 🔺) 2.8 s (+0.05% 🔺) 79 ms (+95.71% 🔺) 2.8 s
Module 132.24 KB (0%) 2.7 s (0%) 71 ms (+51.49% 🔺) 2.8 s
damann commented 4 months ago

Looks good. Thanks so much! Just two tiny things:

  1. There’s something weird happening with the borders of the space switcher not being even.
  2. Could you shave off a few pixels on the right margins of the Apps button so that the hover state looks more even between the label and the arrow? Screenshot 2024-04-26 at 13 33 36 Screenshot 2024-04-26 at 13 45 11
denkristoffer commented 4 months ago

As far as I can tell from the design we don't want the background coloured border to separate the items:

Screenshot 2024-04-29 at 14 47 08
denkristoffer commented 4 months ago

Are you also seeing this weird behaviour with the background cut in half on the hover?

Screenshot 2024-04-29 at 16 54 19
damann commented 4 months ago

Thanks for your attention to detail @denkristoffer! Please still do the following changes @Lelith:

Thanks! ♥️♥️♥️

Lelith commented 4 months ago

@denkristoffer @damann i have addressed all your findings and also went once more over the spacings and polished it some more. ✅ Environment colors: green600 and orange500 ✅ Help icon color: grey700 (also updated all other icons to this color) ✅ Burger icon: size , grey700 ✅ Trial button outline and font color: purple600 ✅ The selected state is currently "inside" the hover state background, in the design it lives below it.

 ❌  Avatar/initials: background grey300, on hover grey400, Initials grey700: @damann and I synced about this and as the default user icon is generated in user_interface we move updating the default icon to the next iteration

cf-remylenoir commented 4 months ago

Nit: What should the focus state look like for the different parts @damann?

Some of them have an extra border color (space/env switcher + avatar) and others only the focus blur.

2024-04-30 16 10 17

Lelith commented 4 months ago

Nit: What should the focus state look like for the different parts @damann?

Some of them have an extra border color (space/env switcher + avatar) and others only the focus blur.

@cf-remylenoir i believe the blue glow should be correct and not the black borders.

cf-remylenoir commented 4 months ago

Something we can follow up on in your absence unless you tackle it before is the loading state.

It seems there are some slight differences in the sizes and alignment of the skeleton elements (i.e. navbar items aren't vertically centered)

Current navigation bar

https://github.com/contentful/forma-36/assets/103024358/807e58a6-095b-4ce7-a2b0-3806c9eb69e3

https://f36-storybook.contentful.com/?path=/story/components-navbar--loading-skeleton

New navigation bar skeleton (switch between Storybook Basic and Loading Skeleton stories, it might not be accurate ?)

https://github.com/contentful/forma-36/assets/103024358/c9920385-cddd-4ba3-a7cf-6c75f9825100

cf-remylenoir commented 4 months ago

I will take care of:

  • ✅ adjusting the Skeleton alignment/sizing
  • ✅ adjusting the navigation height, which is greater than in the design (92px instead of 81px).
  • ✅ adjusting the main container spacings (currently 16px instead of 20px).
  • ✅ adjusting the account button focus (show a ring instead of block).
denkristoffer commented 4 months ago

I think there's still a bit of vertical shift between the skeleton and the navbar items but it's negligible and probably not worth tackling until we change the active state.

cf-remylenoir commented 4 months ago

I think there's still a bit of vertical shift between the skeleton and the navbar items but it's negligible and probably not worth tackling until we change the active state.

This should now be fixed in https://www.npmjs.com/package/@contentful/f36-navbar/v/5.0.0-alpha.1.