Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[website] Fix FlashCode position #42139

Merged
merged 9 commits into from
Jun 2, 2024
Merged

[website] Fix FlashCode position #42139

merged 9 commits into from
Jun 2, 2024

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented May 5, 2024 •
edited
Loading

A quick one I noticed

Before:
SCR-20240505-rbai

After:
SCR-20240505-raxf

Preview: https://deploy-preview-42139--material-ui.netlify.app/

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work website Pages that are not documentation-related, marketing-focused. regression A bug, but worse labels May 5, 2024
@mui-bot
Copy link

mui-bot commented May 5, 2024 •
edited
Loading

Netlify deploy preview

https://deploy-preview-42139--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against f29ea3e

@zannager zannager requested a review from danilo-leal May 6, 2024 16:21
danilo-leal
danilo-leal reviewed May 6, 2024
View reviewed changes
Copy link
Contributor

@danilo-leal danilo-leal left a comment •
edited
Loading

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.

docs/src/components/home/CoreShowcase.tsx Show resolved Hide resolved
docs/src/components/productMaterial/MaterialStyling.tsx Show resolved Hide resolved
@oliviertassinari oliviertassinari force-pushed the fix-material-ui-line-positions branch 3 times, most recently from 2c75661 to 22dc8ed Compare May 26, 2024 22:01
@oliviertassinari
Copy link
Member Author

Oh god, I went down a rabbit hole with this one:

but someone had to 😄

oliviertassinari
oliviertassinari commented May 28, 2024
View reviewed changes
docs/data/base/components/autocomplete/AutocompleteIntroduction/css/index.js
outline: 0px;
outline: 0;
Copy link
Member Author

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.

SCR-20240528-rgpl

danilo-leal
danilo-leal approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@danilo-leal danilo-leal left a 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.

docs/src/components/productMaterial/MaterialStyling.tsx Outdated
@@ -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' });
Copy link
Contributor

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?

Copy link
Member Author

@oliviertassinari oliviertassinari Jun 2, 2024 •
edited
Loading

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.

oliviertassinari
oliviertassinari commented Jun 2, 2024
View reviewed changes
docs/src/components/productBaseUI/BaseUICustomization.tsx Outdated
Comment on lines 123 to 124
const endLine = [26, 93, 84];
const endLine = [31, 93, 84];
Copy link
Member Author

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.

docs/src/components/productBaseUI/BaseUICustomization.tsx
transition: all ease 120ms;

:hover {
background: #9DA8B7;
:where([data-mui-color-scheme='dark']) & {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix stylelint issue

docs/src/components/productBaseUI/BaseUIThemesDemo.tsx
'&:hover:not(.base--selected)': {
background: 'var(--Tab-hoverBackground)',
},

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coding style convention

@oliviertassinari oliviertassinari enabled auto-merge (squash) June 2, 2024 14:58
@oliviertassinari oliviertassinari added the needs cherry-pick The PR should be cherry-picked to master after merge label Jun 2, 2024
@oliviertassinari oliviertassinari merged commit df51eed into mui:next Jun 2, 2024
23 checks passed
@oliviertassinari oliviertassinari deleted the fix-material-ui-line-positions branch June 2, 2024 15:19
joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
@oliviertassinari @joserodolfofreitas
[website] Fix FlashCode position (mui#42139)
ed7f0ea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danilo-leal danilo-leal danilo-leal approved these changes

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work needs cherry-pick The PR should be cherry-picked to master after merge regression A bug, but worse website Pages that are not documentation-related, marketing-focused.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@oliviertassinari @mui-bot @danilo-leal