refactor: switch to Flexbox in announcement bar #5430
Conversation
lex111
added
the
pr: polish
|
✔️ [V2] 🔨 Explore the source changes: 8fa7bbd 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/612cbf800d176f000793a69e 😎 Browse the preview: https://deploy-preview-5430--docusaurus-2.netlify.app |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5430--docusaurus-2.netlify.app/ |
|
Size Change: -72 B (0%) Total Size: 820 kB
ℹ️ View Unchanged
|
| --docusaurus-announcement-bar-height: 30px; | ||
| } | ||
|
|
||
| /* Adjust line-height for proper vertical centering on desktop */ |
There was a problem hiding this comment.
not a fan of using line-height for vertical centering
If user use --docusaurus-announcement-bar-height: 50px; it should remain centered. That's not the case in prod+PR.
I suggest using flex + alignItems/justifyContent center in the navbar content too
There was a problem hiding this comment.
BTW, the close button may not need to use relative positioning.
It is probably possible to render a flex row [placeholder,content,closeButton] using flex: 0 0 50px for elements on the side (both optional elements), and flex-grow: 1 on the center .
Just an idea, probably not really useful to change
There was a problem hiding this comment.
Don't really understand your idea, what kind of placeholder you have in mind? But I fixed vertical alignment, and also applied built-in close class from Infima to close button, it seems to look better, wdyt?
There was a problem hiding this comment.
The placeholder could be an empty div of the same width as the close button that would just push the main content to the center.
There was a problem hiding this comment.
That looks nice overall but the content is not perfectly centered. It's not very visible, but if you increase the close button % width to something more significant, the centering issue is more visible, and this becomes more important to add a placeholder container on the other side.
Also, what about not using % for close button width?
At least for very small viewports, 4% is too small and doesn't really make sense to me, and the close button can touch the edges of the screen.
There was a problem hiding this comment.
I think it would look better if the placeholder used flex-basis: 10px for small widths?
Centering is less important for mobile devices where we want to use more space
There was a problem hiding this comment.
I think it would look better if the placeholder used flex-basis: 10px for small widths?
It doesn't look good, maybe just don't use the placeholder column on mobiles?
There was a problem hiding this comment.
It doesn't look good, maybe just don't use the placeholder column on mobiles?
This is not the result I get 🤪 Only the placeholder should have flex-basis 10 for small screens/mobile, not the close button
There was a problem hiding this comment.
maybe just don't use the placeholder column on mobiles?
I'm fine with removing it too, as long as there's some padding that prevents the main content from touching the edge of the screen (which would be the same as applying that padding by setting a width to this placeholder)
|
👍 |
Motivation
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Preview.
Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)