Skip to content

[Stack] Fix default flexDirection value with responsive prop#33549

Merged
hbjORbj merged 7 commits intomui:masterfrom
hbjORbj:fix/stack-wrong-direction
Jul 20, 2022
Merged

[Stack] Fix default flexDirection value with responsive prop#33549
hbjORbj merged 7 commits intomui:masterfrom
hbjORbj:fix/stack-wrong-direction

Conversation

@hbjORbj
Copy link
Copy Markdown
Contributor

@hbjORbj hbjORbj commented Jul 17, 2022

Problem:

  • If direction prop receives a responsive value such that it doesn't start with the smallest breakpoint, such as direction={{ lg: 'row' }}, flexDirection isn't specified for breakpoints below lg and hence it is set to the default value of flexbox, which is row.

Solution:

  • Set the StackRoot's flexDirection property to ownerState.direction if it is a string, or simply to column.
  • If responsive direction values are provided, this flexDirection property will be overridden.

Codesandboxes:

@hbjORbj hbjORbj added type: bug It doesn't behave as expected. package: material component: Stack The React component. labels Jul 17, 2022
@mui-bot
Copy link
Copy Markdown

mui-bot commented Jul 17, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 8b4e471

@hbjORbj hbjORbj self-assigned this Jul 17, 2022
@oliviertassinari oliviertassinari changed the title [Stack] Correct flexDirection should be used [Stack] Fix default flexDirection value with responsive prop Jul 17, 2022
export const style = ({ ownerState, theme }) => {
let styles = {
display: 'flex',
flexDirection: typeof ownerState.direction === 'string' ? ownerState.direction : 'column',
Copy link
Copy Markdown
Member

@oliviertassinari oliviertassinari Jul 17, 2022

Choose a reason for hiding this comment

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

Would this work? It seems to be what's missing, I assume the typeof ownerState.direction === 'string' ? ownerState.direction logic duplicates with the one in handleBreakpoints?

Suggested change
flexDirection: typeof ownerState.direction === 'string' ? ownerState.direction : 'column',
flexDirection: 'column',

Copy link
Copy Markdown
Contributor Author

@hbjORbj hbjORbj Jul 19, 2022

Choose a reason for hiding this comment

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

I can confirm that you are right. flexDirection: 'column' seems enough. Made the change and added a test.

@hbjORbj hbjORbj requested a review from oliviertassinari July 19, 2022 09:18
Copy link
Copy Markdown
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Nice fix!

@hbjORbj hbjORbj merged commit c384078 into mui:master Jul 20, 2022
@hbjORbj hbjORbj mentioned this pull request Jul 20, 2022
1 task
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: Stack The React component. type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants