contentful / forma-36

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

feat: improve responsiveness for navigation [CFISO-1555] #2805

Closed massao closed 3 weeks ago

massao commented 1 month ago

Purpose of PR

Mobile S:

image

Mobile Medium

image

Desktop S

image

Desktop M

image
vercel[bot] commented 1 month 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 4, 2024 8:37am
changeset-bot[bot] commented 1 month ago

⚠️ No Changeset found

Latest commit: 92b6ffff6f07b50f06f68c4a46dca09f0441dd69

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

cf-remylenoir commented 1 month ago

Could you please add a story for the mobile view or see if we can fix the Storybook viewport feature? I tried using the Storybook viewport feature to get the responsiveness but it does not seem to work 🤔

2024-06-25 15 58 19

massao commented 1 month ago

Could you please add a story for the mobile view or see if we can fix the Storybook viewport feature? I tried using the Storybook viewport feature to get the responsiveness but it does not seem to work 🤔

Added a With Responsiveness Story to make it easier, and not have to change all the others

damann commented 1 month ago
Screenshot 2024-07-01 at 17 12 14

Once the secondary navigation icons are hidden:

Not specified in the design yet:

Lelith commented 1 month ago

@denkristoffer

denkristoffer commented 1 month ago
* minimum gap is added but as we can not use container based queries (because of our outdated emotion version) it's super hard to find the sweet spot of width for the switcher.

@Lelith Just guessing but if something is not supported in emotion 10 it would probably be due to the version of stylis. Maybe we can force it to the latest version and container queries could work. Do you want to try it?

Looks like we need to use stylis v4.1.3 or newer https://github.com/thysultan/stylis/pull/304

cf-remylenoir commented 1 month ago
* minimum gap is added but as we can not use container based queries (because of our outdated emotion version) it's super hard to find the sweet spot of width for the switcher.

@Lelith Just guessing but if something is not supported in emotion 10 it would probably be due to the version of stylis. Maybe we can force it to the latest version and container queries could work. Do you want to try it?

Looks like we need to use stylis v4.1.3 or newer thysultan/stylis#304

Actually yes, that could work

Lelith commented 1 month ago

@denkristoffer @cf-remylenoir we can try it for sure, i am just at a loss where all the configuration and packages are defined for it, they are not noted in any of the package.json files for forma?

denkristoffer commented 1 month ago

@denkristoffer @cf-remylenoir we can try it for sure, i am just at a loss where all the configuration and packages are defined for it, they are not noted in any of the package.json files for forma?

We have to add an "overrides" key in package.json. Docs: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#overrides

Lelith commented 1 month ago

@cf-remylenoir @denkristoffer synced with Daniel and we decided to increase all icon sizes for the mobile views. had to do some overwrites for that as we do not have media queries in JS and the icon sizes are not a token, i had to handwrite them. Should we add these sizes also as tokens?

damann commented 1 month ago

@cf-remylenoir @denkristoffer synced with Daniel and we decided to increase all icon sizes for the mobile views. had to do some overwrites for that as we do not have media queries in JS and the icon sizes are not a token, i had to handwrite them. Should we add these sizes also as tokens?

Does that mean they don't use Icon size="medium"?

Lelith commented 3 weeks ago

@damann the icon size properties are not setup responsive, they have a single fixed size. If we want to support more responsive designs in the future, we should establish a pattern for Icon Sizes soon