search

mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
92.37k stars 31.81k forks source link

[material-ui][Button] Negative margin causes start icon and end icon to be outside parent container #41103

Open JaceInglis opened 4 months ago

JaceInglis commented 4 months ago

Steps to reproduce

Steps:

  1. Visit the sandbox reproduction
  2. Open dev tools (Inspect Element) and notice that when you hover over the button that the startIcon is outside the button container
  3. Also notice how when there is no padding on the button the Icon will get cutoff due to the negative margin

Current behavior

Current behaviour: Button Icons fall outside of the parent container due to negative margin.

Expected behavior

Expected behavior: Button Icons stay inside their parent container.

Context

I want to be able to have no padding on my button and still have the content stay inside the components container.

Issue created for: https://github.com/mui/material-ui/pull/40956#pullrequestreview-1869925897

Link to PR: https://github.com/mui/material-ui/pull/40956

Your environment

Using Chrome

npx @mui/envinfo ``` System: OS: Linux 6.1 Debian GNU/Linux 12 (bookworm) 12 (bookworm) Binaries: Node: 20.9.0 - /usr/local/bin/node npm: 9.8.1 - /usr/local/bin/npm pnpm: 8.10.2 - /usr/local/share/npm-global/bin/pnpm Browsers: Chrome: Not Found npmPackages: react: 16.8.6 => 16.8.6 react-dom: 16.8.6 => 16.8.6 typescript: 3.3.3 => 3.3.3 ```

Search keywords: button negative margin start end icon

danilo-leal commented 4 months ago

Hey @JaceInglis, thanks for opening the issue! For the sake of curiosity, do you mind elaborating more on your use case? Why do you want to remove the button's padding entirely?

JaceInglis commented 4 months ago

Hey @danilo-leal I want to be able to have a full text button that aligns with the rest of my text other wise text buttons look like they are missaligned due to there internal padding.

Without removed padding image

With removed padding (issue can be seen here) image

Expected (If issue of Icon falling outside of parent container is fixed) image

danilo-leal commented 4 months ago

I'd generally advise against messing with spacing from inside the component — maybe a different approach would work for you, and the Card's page introduction demo seems to be pretty close to your use case. Take a look there and let me know if something similar works for you!

JaceInglis commented 4 months ago

Hey @danilo-leal the component above doesn't utilize the card component. Putting the removal of the padding aside, the icon within the button still visibly extends beyond the parent container. Could you share your perspective on why addressing this isn't preferred? If the concern is the slight spacing adjustment between the icon and text, perhaps we could consider maintaining the spacing by adding an equivalent amount on the opposite side of the icon. Your insights on this matter would be appreciated.

danilo-leal commented 4 months ago

The biggest downside that I see of affecting the inner spacing of the Button component is messing with the style of the different states, primarily the hover and focus states. The demo I linked above is just an illustration of how you can fix the problem by wrapping the content and the button in containers with different padding instead of tweaking the components' inner spacing—no need to necessarily be a Card; you can do it with a simple Box!

Check this out: I put together a quick CodeSandbox using just the Box component and padding to illustrate everything I said above! Let me know if it's helpful!

JaceInglis commented 4 months ago

@danilo-leal Your sandbox reproduction uses button size small which actually fixes the issue. image

If you try using medium you will not be able to make a working example without

A. the icon falling outside the line or B. using small pixel adjustments to fix the deeper underlying issue in the button component

For clarification this is the issue I'm trying to solve. image image image

JaceInglis commented 4 months ago

@danilo-leal Just making sure you saw this ^