-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
chore: reduce Searchbox button layout shift #7358
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
724d0b7
to
dabe8a2
Compare
Hey @rjborba Im adding a skeleton component on a sibling PR, would you mind using it once that PR gets merged? That would even improve the rendering IMO. Also looking forward for the SSR support, or at least even if not being SSR compatible, if you could move all window API calls to Hooks, that would already do it. Right now React is bailing out initial page pre-rendering due to the component using window APIs on mount. I would recommend simply making all these aspects of the component behind Hooks/asynchronous. |
Lighthouse Results
|
@rjborba you can rebase now :) |
dabe8a2
to
0d6467a
Compare
@ovflowd Hi! I couldn't find the PR you mentioned, but I found a Skeleton component and I'm using it. Can you please check if this is the right component/behavior? Thank you! |
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.
We are loading Instrument Sans
font on the pages additionally for the search box button, which causes a slight layout shift while loading process
IMO we can directly use the Open Sans
we use on the pages. Adding typography information to the configuration looks to be sufficient;
export const themeConfig = {
typography: {
'--font-primary': 'var(--font-open-sans)',
},
colors: {
...
.searchButtonSkeleton { | ||
@apply flex-grow; | ||
} | ||
|
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.
.searchButtonSkeleton { | |
@apply flex-grow; | |
} | |
.searchButtonSkeleton { | |
@apply my-px | |
mr-2 | |
flex-grow | |
rounded-xl; | |
&:empty { | |
@apply h-10; | |
} | |
} |
We can make the height and radius of this area a little more similar to the search box button
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.
Ah, my bad 😅 The given height is not visible because the order of styles changed after the build, can you update it with a more specific class name?
Something like;
.searchButtonSkeleton[data-inline-skeleton]
or span.searchButtonSkeleton
should work
@canerakdas All your requests should be now fixed. Would you mind to double check? Thank you! |
Description
This is a small tweak to prevent the Cumulative Layout Shift caused by the lazily loaded Search Button Component. The ‘final’ solution would be to support SSR in orana-ui-components, which we are actively working on, but still no ETA.
Validation
Compared to the old behavior, where the items on the right-hand side of the navbar would shift positions, now the items remain fixed. The only change is that the SearchBox is displayed in place of the previously empty space
Related Issues
No issues were created for it
Check List
npm run format
to ensure the code follows the style guide.npm run test
to check if all tests are passing.npx turbo build
to check if the website builds without errors.