[system] Fix breakpoint order #26773
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
renatoagds
changed the title
Details of bundle changes.Comparing: 4f45597...a976d06 Details of page changes
|
oliviertassinari
changed the title
|
We worked on a similar problem in #23380. Maybe we need to use |
| @@ -28,3 +28,33 @@ describe('breakpoints', () => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('handleBreakpoints', () => { | |||
There was a problem hiding this comment.
This is a private module. Could we reorganize the test case to cover a public API that reproduces the bug? This is important because our current focus with the system is to make it works and right, fast will come after and likely require to reorganize the internal API.
There was a problem hiding this comment.
@oliviertassinari Just one public api it's worth? Because I reproduced it at least on two APIs (spacing, flexbox).
There was a problem hiding this comment.
Ideally, with the sxFunction converter, otherwise, style, https://github.com/mui-org/material-ui/blob/c795ca0822e2816a630a5654d5471fa04521d23c/packages/material-ui-system/src/flexbox.js#L4
@oliviertassinari Do you think that we should bring the "same" code from next to master? That's some point different there? |
|
@renatoagds Oh, I didn't realize we are on master. Please rebase on next. We haven't been working on master (v4) for 12+ months. |
|
@oliviertassinari this is a bug that's happening on latest release (v4), I could confirm if happens on v5, but for v4 the branch that is being used is master or not? |
|
v4 -> master branch, v5 -> next branch. |
|
I've just checked and it works with v5. |
oliviertassinari
added
status: waiting for author
|
@mnajdova Not completely. I can reproduce the issue with: import * as React from "react";
import { styled } from "@material-ui/core/styles";
import Grid from "@material-ui/core/Grid";
import Paper from "@material-ui/core/Paper";
import Box from "@material-ui/core/Box";
const Item = styled(Paper)(({ theme }) => ({
...theme.typography.body2,
padding: theme.spacing(1),
textAlign: "center",
color: theme.palette.text.secondary
}));
export default function RowAndColumnSpacing() {
return (
<Box sx={{ width: "100%" }}>
<Grid container columnSpacing={{ lg: 5, sm: 1 }}>
<Grid item xs={6}>
<Item>1</Item>
</Grid>
<Grid item xs={6}>
<Item>2</Item>
</Grid>
</Grid>
</Box>
);
}https://codesandbox.io/s/priceless-cohen-q14i7?file=/src/App.js |
oliviertassinari
added
bug 🐛
|
@oliviertassinari so, can I follow with this fix for v4 (fixing the comment about the test) and prepare a new one for v5? |
github-actions
bot
added
the
PR: out-of-date
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When you use an prop as object, following the breakpoint approach, if the object do not follow the size order:
p={{ md: .., sm: ..., xs: ... }}It won't work, since the priority comes right to left.
An example could be found here: https://codesandbox.io/s/keys-order-mui-v8qjb?file=/src/App.js
Just open the sandbox and resize the browser panel, you will see that yellow box never changes the padding.
This is an bug dectected after use eslint sort-keys rule: https://eslint.org/docs/rules/sort-keys