Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: improve responsiveness of navbar #2934

Merged
merged 13 commits into from
Nov 26, 2024
Merged

feat: improve responsiveness of navbar #2934

merged 13 commits into from
Nov 26, 2024

Conversation

Lelith
Copy link
Collaborator

@Lelith Lelith commented Nov 18, 2024 •
edited
Loading

Purpose of PR

  • exports media queries variables from the package

  • enables hiding promo navigation on smaller screens

  • changes the text overflow behavior and width of the Navbar.Switcher component. Before the text was just cut off and there was no max-width on the switcher
    before:
    image

after:
image

  • Enables two different breakpoints of when to start showing the mobile navigation over the main navigation
    image

@Lelith Lelith requested review from damann and a team as code owners November 18, 2024 11:33
@Lelith Lelith requested review from andipaetzold and removed request for a team November 18, 2024 11:33
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 18, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 565c4df

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.

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

💥 An error occurred when fetching the changed packages and changesets in this PR 
Some errors occurred when validating the changesets config:
The package "@contentful/f36-components" depends on the ignored package "@contentful/f36-avatar", but "@contentful/f36-components" is not being ignored. Please add "@contentful/f36-components" to the `ignore` option.
The package "@contentful/f36-components" depends on the ignored package "@contentful/f36-image", but "@contentful/f36-components" is not being ignored. Please add "@contentful/f36-components" to the `ignore` option.
The package "@contentful/f36-components" depends on the ignored package "@contentful/f36-header", but "@contentful/f36-components" is not being ignored. Please add "@contentful/f36-components" to the `ignore` option.

@vercel Vercel
Copy link

vercel bot commented Nov 18, 2024 •
edited
Loading

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

Name Status Preview Updated (UTC)
forma-36 ✅ Ready (Inspect) Visit Preview Nov 26, 2024 6:39am

@Lelith Lelith requested review from veu and cf-remylenoir and removed request for damann and andipaetzold November 18, 2024 11:33
@Lelith Lelith changed the title feat: export media queries feat: improve responsiveness of navbar Nov 18, 2024
@Lelith Lelith requested a review from veu November 18, 2024 13:00
veu
veu previously approved these changes Nov 18, 2024
View reviewed changes
cf-remylenoir
cf-remylenoir previously approved these changes Nov 18, 2024
View reviewed changes
@Lelith Lelith marked this pull request as draft November 20, 2024 16:19
@Lelith Lelith marked this pull request as ready for review November 25, 2024 13:17
@Lelith Lelith marked this pull request as draft November 25, 2024 13:21
@Lelith Lelith marked this pull request as ready for review November 25, 2024 14:19
@Lelith Lelith requested review from cf-remylenoir and veu November 25, 2024 14:19
@Lelith Lelith dismissed stale reviews from cf-remylenoir and veu November 25, 2024 14:20

significant changes since the approval

cf-remylenoir
cf-remylenoir reviewed Nov 25, 2024
View reviewed changes
packages/components/navbar/src/Navbar.tsx
Comment on lines +35 to +36
/** breakpoint to determine when to show the mobile navigation */
mobileNavigationBp?: 'small' | 'medium';
Copy link
Collaborator

@cf-remylenoir cf-remylenoir Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the deciding factor to select one or the other? Is it the number of nav items (i.e. the organization settings)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, for the organization settings navigation, there is already not enough space for the switcher with the main navigation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can revisit this logic once we support container queries, we should be able to make it fully dynamic based on the surrounding elements 🤞

cf-remylenoir
cf-remylenoir approved these changes Nov 25, 2024
View reviewed changes
Copy link
Collaborator

@cf-remylenoir cf-remylenoir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA looking good 👍

@Lelith Lelith force-pushed the fix/navbar_responsiveness_F36-1444 branch from 373d90d to 565c4df Compare November 26, 2024 06:30
@Lelith Lelith merged commit bfb427a into next Nov 26, 2024
11 checks passed
@Lelith Lelith deleted the fix/navbar_responsiveness_F36-1444 branch November 26, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cf-remylenoir cf-remylenoir cf-remylenoir approved these changes

@veu veu Awaiting requested review from veu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Lelith @veu @cf-remylenoir