-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[website] Fix FlashCode position #42139
[website] Fix FlashCode position #42139
Conversation
Netlify deploy previewhttps://deploy-preview-42139--material-ui.netlify.app/ Bundle size report |
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.
Cool! Left a couple of remarks. I'm also touching on these components somewhat significantly at another PR (#38604). Lastly, I wonder if this is really a regression? It sounds too severe as it's non-functional and non-blocking—a bug rings as more reasonable.
2c75661
to
22dc8ed
Compare
baf4435
to
6d40b5c
Compare
outline: 0px; | ||
outline: 0; |
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.
This is not detected by the stylelint parser, I created an issue for them: hudochenkov/postcss-styled-syntax#31.
6d40b5c
to
94476db
Compare
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.
Cool—looks good! Lots of non-immediately related changes to the PR's goal, though... the outline stuff and Base UI demo improvements could've been saved for a different, dedicated PR.
@@ -151,7 +145,7 @@ export default function MaterialStyling() { | |||
} | |||
React.useEffect(() => { | |||
if (infoRef.current) { | |||
infoRef.current.scroll({ top: scrollTo[index], behavior: 'smooth' }); | |||
infoRef.current.scroll({ top: scrollTo[index] * 0.75 * 16 * 1.5 - 3, behavior: 'smooth' }); |
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.
Where do these magic numbers come from?
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.
Great point. I have added comments and refactored the logic: 31da76b.
const endLine = [26, 93, 84]; | ||
const endLine = [31, 93, 84]; |
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.
Not covering the right lines.
transition: all ease 120ms; | ||
|
||
:hover { | ||
background: #9DA8B7; | ||
:where([data-mui-color-scheme='dark']) & { |
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.
Fix stylelint issue
'&:hover:not(.base--selected)': { | ||
background: 'var(--Tab-hoverBackground)', | ||
}, | ||
|
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.
Coding style convention
A quick one I noticed
Before:
After:
Preview: https://deploy-preview-42139--material-ui.netlify.app/