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.

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

Improve the layout responsiveness of SubdomainNavBar #644

Merged
merged 5 commits into from
Jul 16, 2024

Conversation

danielguillan
Copy link
Contributor

@danielguillan danielguillan commented Jul 10, 2024

Summary

This pull request improves the layout responsiveness of SubdomainNavBar. Fixes #581.

🔗 Storybook

List of notable changes:

  • Uses the small button variant for CTAs.
  • Makes the title text responsive by relying on the Text component instead of fixed CSS values.
  • Adjusts gap in the title area at different viewport sizes.
  • Adapts link paddings at different viewport sizes.

What should reviewers focus on?

  • Test the changes in Storybook at different viewport sizes

Contributor checklist:

  • All new and existing CI checks pass
  • Tests prove that the feature works and covers both happy and unhappy paths
  • Any drop in coverage, breaking changes or regressions have been documented above
  • New visual snapshots have been generated / updated for any UI changes
  • All developer debugging and non-functional logging has been removed
  • Related issues have been referenced in the PR description

Reviewer checklist:

  • Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
  • Check that tests prove the feature works and covers both happy and unhappy paths
  • Check that there aren't other open Pull Requests for the same update/change

Screenshots:

Before After
image image

Copy link

changeset-bot bot commented Jul 10, 2024

🦋 Changeset detected

Latest commit: c91771b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@primer/react-brand Patch
@primer/brand-primitives Patch
@primer/brand-e2e Patch
@primer/brand-fonts Patch
@primer/brand-config Patch
@primer/brand-storybook Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link
Contributor

github-actions bot commented Jul 10, 2024

🟢 No design token changes found

Copy link
Contributor

github-actions bot commented Jul 10, 2024

🟢 No visual differences found

Our visual comparison tests did not find any differences in the UI.

@danielguillan danielguillan force-pushed the danielguillan/improve-subdomainnavbar-layout branch from 4969b61 to 4c4e0ea Compare July 11, 2024 06:45
@danielguillan danielguillan marked this pull request as draft July 11, 2024 06:46
Copy link
Collaborator

@rezrah rezrah left a comment

Choose a reason for hiding this comment

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

This is great, thanks @danielguillan 🙌

FYI, the VRT test is failing for one of the stories you mentioned. Let me know if you'd like a hand fixing it.

Copy link

@simmonsjenna simmonsjenna left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@danielguillan danielguillan merged commit b3f1f36 into main Jul 16, 2024
17 checks passed
@danielguillan danielguillan deleted the danielguillan/improve-subdomainnavbar-layout branch July 16, 2024 08:28
@primer-css primer-css mentioned this pull request Jul 16, 2024
@rezrah rezrah mentioned this pull request Jul 17, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 [BUG] - SubdomainNavBar has wrapping/overflow issues
3 participants