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.

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

[RFC] Add experimental flag to use flexbox gap in Stack #33754

Closed
siriwatknp opened this issue Aug 2, 2022 · 6 comments · Fixed by #36404
Closed

[RFC] Add experimental flag to use flexbox gap in Stack #33754

siriwatknp opened this issue Aug 2, 2022 · 6 comments · Fixed by #36404
Labels
component: Stack The React component. new feature New feature or request package: system Specific to @mui/system RFC Request For Comments

Comments

@siriwatknp
Copy link
Member

siriwatknp commented Aug 2, 2022

Motivation

  • The browser support is promising at 91.87% https://caniuse.com/?search=flexbox%20gap
  • The current approach uses margin via the selector '& > :not(style) + :not(style)' which is not a good DX when it comes to override.
    <Stack>
      <Button sx={{ mt: 1 }}>Test</Button> // the margin-top does not win.
    </Stack>
  • Using gap reduces the complexity of the code because it works for both row and column direction.

It would fix:

API

Adding a flag useFlexGap to Stack to switch the implementation from margin to flexbox gap:

<Stack spacing={2}>...</Stack> // '& > :not(style) + :not(style)': { marginTop: 16px };

<Stack spacing={2} useFlexGap>...</Stack> // { gap: 16px };

To switch all stacks to use gap, do it via the theme:

createTheme({
  components: {
    MuiStack: {
      defaultProps: {
        useFlexGap: true,
      }
    }
  }
})

Note: I used to think about using CSS feature query @support but it does not work for gap because the property is identical to CSS Grid gap which supports in most browser already.

@siriwatknp siriwatknp added new feature New feature or request component: Stack The React component. labels Aug 2, 2022
@oliviertassinari oliviertassinari added discussion RFC Request For Comments package: system Specific to @mui/system labels Aug 21, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 21, 2022

Regarding the name of the prop enableGap, I assume that we will eventually remove the prop in v6 to make it the default as we update the browser support. No objections with this name. Though, "enable" sounds like it's additive but it's replacing the existing implementation. Maybe useFlexGap would be more descriptive.


With this change, it seems that we could have a follow-up PR to solve: #28035. I have done a quick proof of concept: https://codesandbox.io/s/vigilant-mcclintock-pivrt3?file=/demo.tsx

Screenshot 2022-08-21 at 14 07 18

I'm even wondering if the wrapping behavior shouldn't be the default, to benchmark with the other Stack components: https://mui-org.notion.site/Stack-component-ef1f3428178146129edd78c5d667ccac.

With this change, it seems that we could have a follow-up PR to solve: #33155 & #33810.

@siriwatknp
Copy link
Member Author

No object with the name useFlexGap, it sounds better than my proposal!

@siriwatknp
Copy link
Member Author

The PR is ready for review! #36404

@lnhrdt
Copy link

lnhrdt commented Apr 4, 2023

I just tried this out in the latest release. It works great and simplifies a lot of my components. Thank you so much for this new feature!

@LvChengbin
Copy link

Why not keep both spacing and gap? They have different effects. The spacing just adds column-gap or row-gap depends on the flex direction, however gap adds gap.
Replacing spacing with gap should be a breaking change because some people may be using spacing + margin-bottom to make the gap effects.

@siriwatknp
Copy link
Member Author

The spacing just adds column-gap or row-gap depends on the flex direction, however gap adds gap.

gap is composed of row-gap | column-gap, see https://developer.mozilla.org/en-US/docs/Web/CSS/gap#formal_syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Stack The React component. new feature New feature or request package: system Specific to @mui/system RFC Request For Comments
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants