mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.43k stars 32.16k forks source link

[Dialog] `DialogActions` spacing breaks when elements aren't the same type #36947

Closed mikew closed 12 months ago

mikew commented 1 year ago

Duplicates

Latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/pensive-ride-4ntm36?file=/demo.tsx

Steps:

  1. Add DialogActions
  2. Add a <Button>
  3. Add a <Button component="a">

Current behavior 😯

Since the CSS selector is :first-of-type, if the elements have a different type, spacing between them won't work.

Expected behavior 🤔

There would be spacing between the actions in a DialogActions component

Context 🔦

One of our buttons is a "Close" button, the other is a link to download an asset. Since it's a link, it should be an a tag, not some random onClick handler.

Your environment 🌎

npx @mui/envinfo ``` System: OS: Linux 6.2 Arch Linux Binaries: Node: 12.14.0 - [redacted]/v12.14.0/bin/node Yarn: 1.22.18 - [redacted]/yarn npm: 6.13.4 - [redacted]/node_modules/.bin/npm Browsers: Chrome: Not Found Firefox: Not Found npmPackages: @emotion/react: ^11.10.6 => 11.10.6 @emotion/styled: ^11.10.6 => 11.10.6 @mui/base: 5.0.0-alpha.125 @mui/core-downloads-tracker: 5.12.0 @mui/icons-material: ^5.11.16 => 5.11.16 @mui/material: ^5.12.0 => 5.12.0 @mui/private-theming: 5.12.0 @mui/styled-engine: 5.12.0 @mui/system: 5.12.0 @mui/types: 7.2.4 @mui/utils: 5.12.0 @types/react: ^17.0.3 => 17.0.3 react: ^17.0.0 => 17.0.2 react-dom: ^17.0.0 => 17.0.2 styled-components: ^5.0.1 => 5.0.1 typescript: ^4.5.4 => 4.5.5 ```
mj12albert commented 1 year ago

@mikew Thanks for reporting this - we can only use :first-of-type since :first-child isn't safe for SSR

In your case you could manually reapply the spacing using a different selector, example: https://codesandbox.io/s/dialog-actions-spacing-v0iru2?file=/src/App.tsx

mikew commented 1 year ago

dang, I kinda can't wait until we can move past css-in-js 😅 . Thanks for the tip!

ZeeshanTamboli commented 1 year ago

I think we've found a solution mentioned here, so we can close this issue.

mycroes commented 1 year ago

I think we've found a solution mentioned here, so we can close this issue.

There's no solution, there's a workaround. I propose to at least reopen this issue so the fact it's broken can be tracked.

ZeeshanTamboli commented 1 year ago

I apologize for closing this issue . This is definitely a workaround so I reopened the issue.

One approach might involve looping through the children, using React context or other API to find their positions, and applying styling similar to what's being done for ButtonGroup in #38520. But DialogActions may not contain only Material UI Buttons, but other elements as well so this would not be ideal.

mycroes commented 1 year ago

I do think that the solution would work. Basically you only need to add a data attribute to the first element and change the selector to :not(data-first-dialog-action-child).

I'm unaware of a solution to cloning/detecting non-rendering elements (<Tooltip />, <Dialog />), but I guess that's not new for the MUI team?

mikew commented 1 year ago

To work around this, we just wrapped each element in a <span> or <div>. Can't remember which, just as long as it's the same element for each

JulianIV commented 1 year ago

Is there a reason why gap: 8px won't work?

ZeeshanTamboli commented 1 year ago

Is there a reason why gap: 8px won't work?

Because of browser support for flexbox layout's gap property. See https://github.com/mui/material-ui/pull/35104#issuecomment-1312475844. This could work in v6 when we update Safari browser support. 👍