-
Notifications
You must be signed in to change notification settings - Fork 536
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
Hidden: Refactor the component to use the new getBreakpointsDeclarations
util function and add missing stories and tests
#2729
Conversation
🦋 Changeset detectedLatest commit: 1f9bae0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
getBreakpointsDeclarations
util function to make it SSR friendly
c40924b
to
05da78f
Compare
05da78f
to
978af5a
Compare
getBreakpointsDeclarations
util function to make it SSR friendlygetBreakpointsDeclarations
util function and add missing stories and tests
@mperrotti Oh right!! This is on the new canvas! Sorry I missed that. Actually, this is a bug (more like an uncovered case) in UnderlineNav! (In that example, I used UnderlineNav for the navigation). It is a coincidence that I am synchronously working on it. |
37fd3b8
to
90786c4
Compare
9c718cc
to
da4353b
Compare
@joshblack just renamed a few things to reduce confusion around breakpoints/ranges and refactored I'd like to merge this to move forward with the accessibility eng review for PageHeader - but please let me know if you have any concern - happy to address them in a following PR if they are not blocking here - thanks so much again for your suggestions 🙏🏻 PR still shows the feedback you left (requested changes) as blocking for merge. If all look good to you, could I please get ✅ as well? 🙌🏻 Thanks!! |
* Add visual ordering and pageLayout example * Mock IntersectionObserver for <ThemeProvider> in story render * remove unused components * add TitleArea order * add changeset * update the snapshot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Closes https://github.com/github/primer/issues/1664
Hidden
component is being used inPageHeader
and working as expected. After improving the way we manage the responsive visibility onPageHeader
viagetBreakpointDeclarations
utility function to reduce the layout shift in SSR - I applied the same strategy toHidden
component. I refactored it to use thegetBreakpointDeclarations
function rather thanuseResponsiveHook
as this is something can be achieved with CSS as well.I also wrote unit tests, added docs and stories.
Screenshots
No visual changes. All examples are available on storybook and docs.
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.