-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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: upgrade Infima to alpha.34 #5666
Conversation
Size Change: +56 B (0%) Total Size: 837 kB ℹ️ View Unchanged
|
✔️ [V2] 🔨 Explore the source changes: 9586fa7 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61605af0fd8eb20008217db8 😎 Browse the preview: https://deploy-preview-5666--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5666--docusaurus-2.netlify.app/ |
(imageClassName ? ( | ||
<div className={imageClassName}>{themedImage}</div> | ||
) : ( | ||
<>{themedImage}</> |
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.
do we really need to handle this case? we don't have it in our codebase
I think this component is quite messy and it's only used twice, for mobile/desktop sidebars with exact same props:
<Logo
className="navbar__brand"
imageClassName="navbar__logo"
titleClassName="navbar__title"
/>
What about replacing this with <NavbarLogo>
and apply classes inside of it?
I don't think this component is meant to be used in any other place so I'd rather make the coupling to navbar more explicit
(this is technically a theme breaking but I think we should do it. Can be in another PR though)
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.
Yes, this component has another use case:
{hideOnScroll && <Logo tabIndex={-1} className={styles.sidebarLogo} />} |
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.
thanks, didn't see it
still, not a fan at all of adding/removing the extra div when adding/removing the imageClassName
=> this is not explicit and not an expected side-effect of using this prop: it should just apply a className or not
Also, the 3 places where this comp is used look exactly the same. I don't really understand why this DocSidebar thing can't use the same classes as the navbar.
Going to merge now but I think there's some additional cleanup we could do on this comp
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.
LGTM, didn't see any issue apart some useless code in Logo, but cleanup can be done in a separate PR
Motivation
Of the important changes, navbar logo is wrapped in separate div.
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.)