forma-36 icon indicating copy to clipboard operation
forma-36 copied to clipboard

[DO NOT MERGE] feat: navigation light mode

Open Lelith opened this issue 1 year ago • 14 comments

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.

Lelith avatar Apr 25 '24 08:04 Lelith

⚠️ 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

changeset-bot[bot] avatar Apr 25 '24 08:04 changeset-bot[bot]

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

vercel[bot] avatar Apr 25 '24 08:04 vercel[bot]

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

github-actions[bot] avatar Apr 25 '24 12:04 github-actions[bot]

Looks good. Thanks so much! Just two tiny things: 2. There’s something weird happening with the borders of the space switcher not being even. 3. 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

damann avatar Apr 26 '24 11:04 damann

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 avatar Apr 29 '24 12:04 denkristoffer

Are you also seeing this weird behaviour with the background cut in half on the hover? Screenshot 2024-04-29 at 16 54 19

denkristoffer avatar Apr 29 '24 14:04 denkristoffer

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

  • Environment colors: green600 and orange500
  • Help icon color: grey700
  • Avatar/initials: background grey300, on hover grey400, Initials grey700
  • Burger icon: size , grey700
  • Trial button outline and font color: purple600

Thanks! ♥️♥️♥️

damann avatar Apr 30 '24 08:04 damann

@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

Lelith avatar Apr 30 '24 12:04 Lelith

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

cf-remylenoir avatar Apr 30 '24 13:04 cf-remylenoir

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.

Lelith avatar Apr 30 '24 14:04 Lelith

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 avatar Apr 30 '24 15:04 cf-remylenoir

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).

cf-remylenoir avatar May 02 '24 08:05 cf-remylenoir

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.

denkristoffer avatar May 02 '24 13:05 denkristoffer

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.

cf-remylenoir avatar May 02 '24 14:05 cf-remylenoir