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

[ButtonGroup] Non-homogeneous border #25280

Closed
2 tasks done
zhexuan-flux opened this issue Mar 9, 2021 · 6 comments
Closed
2 tasks done

[ButtonGroup] Non-homogeneous border #25280

zhexuan-flux opened this issue Mar 9, 2021 · 6 comments
Labels
component: ButtonGroup The React component. out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future)

Comments

@zhexuan-flux
Copy link

When grouping two buttons inside the component, borders of the two buttons overlap and result in some low quality detail

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The vertical line that divides the two buttons has some lighter pixels where it meets the button outline.

Screen Shot 2021-03-09 at 10 09 55 PM

Expected Behavior 🤔

When placing two buttons within a component, their border should be connected smoothly and consistently, as the following screenshot:

Screen Shot 2021-03-09 at 10 06 02 PM

In our app, we manually set the style by putting "border-right: 1px" for the left button, and "margin-left: 0px" for the right button. Before, right button has "margin-left: -1px" and there is no border-right for left button, which result in the current behavior above

Steps to Reproduce 🕹

Steps:

  1. Code two buttons in component. For example, we have:
        <ButtonGroup className={classes.starButtonGroup}>
            <Button
                    aria-label="reduce"
                    className={classes.starButton}
                    onClick={handleClickStarDocument}
                >
                    {starred ? (
                        <StarIcon fontSize="small" className={classes.starButtonIcon} />
                    ) : (
                        <StarBorderIcon fontSize="small" className={classes.starButtonIcon} />
                    )}
                    Star
            </Button>
            <Button aria-label="increase" className={classes.starCountButton}>
                <Typography display="inline" variant="body2">
                    {starCount}
                </Typography>
            </Button>
        </ButtonGroup>
  1. Render it and should see the bug in page

Context 🔦

Your Environment 🌎

React app with nvm -> 12.16.2

`npx @material-ui/envinfo`
  Don't forget to mention which browser you used.
  Output from `npx @material-ui/envinfo` goes here.
@zhexuan-flux zhexuan-flux added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Mar 9, 2021
@oliviertassinari
Copy link
Member

Interesting. It's because we collapse the following with margin-left: -1px

Screenshot 2021-03-09 at 15 56 37

@oliviertassinari oliviertassinari added the component: ButtonGroup The React component. label Mar 9, 2021
@oliviertassinari oliviertassinari changed the title Low quality detail in default ButtonGroup [ButtonGroup] Non-homogeneous border Mar 9, 2021
@oliviertassinari oliviertassinari added low priority and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Mar 9, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 9, 2021

End-users need to zoom in x2 to start being able to see the issue. I'm hesitating to add the "won't fix" flag as there is likely no other option than to remove the opacity from the border, which has other downsides, probably more important ones. So, it would be a tradeoff.

We had to solve a similar problem in #9229, but it was a lot more noticeable:

https://github.com/mui-org/material-ui/blob/cce45deec3125c1f0dd6c1c74616b5e63b027026/packages/material-ui/src/TableCell/TableCell.js#L59-L66

@oliviertassinari
Copy link
Member

Actually, looking closer at https://material.io/components/buttons, it's not this obvious. The Material Design specification document the outlined color as grey:

Screenshot 2021-03-10 at 16 08 28

and support.google.com would implement it as follow:

Screenshot 2021-03-10 at 16 08 41

with plain border color. I recall we discussed this problem in #15242. It might be time to resume it.

If we want to use a plain color, this would do it.

diff --git a/packages/material-ui/src/Button/Button.js b/packages/material-ui/src/Button/Button.js
index 1a4ec32485..ccd6b88328 100644
--- a/packages/material-ui/src/Button/Button.js
+++ b/packages/material-ui/src/Button/Button.js
@@ -5,7 +5,7 @@ import { deepmerge } from '@material-ui/utils';
 import { unstable_composeClasses as composeClasses } from '@material-ui/unstyled';
 import experimentalStyled, { shouldForwardProp } from '../styles/experimentalStyled';
 import useThemeProps from '../styles/useThemeProps';
-import { alpha } from '../styles/colorManipulator';
+import { alpha, emphasize } from '../styles/colorManipulator';
 import ButtonBase from '../ButtonBase';
 import capitalize from '../utils/capitalize';
 import buttonClasses, { getButtonUtilityClass } from './buttonClasses';
@@ -190,7 +190,7 @@ const ButtonRoot = experimentalStyled(
     ...(styleProps.variant === 'outlined' &&
       styleProps.color !== 'inherit' && {
         color: theme.palette[styleProps.color].main,
-        border: `1px solid ${alpha(theme.palette[styleProps.color].main, 0.5)}`,
+        border: `1px solid ${emphasize(theme.palette[styleProps.color].main, 0.45)}`,
       }),
     ...(styleProps.variant === 'contained' && {
       color: theme.palette.getContrastText(theme.palette.grey[300]),
diff --git a/packages/material-ui/src/ButtonGroup/ButtonGroup.js b/packages/material-ui/src/ButtonGroup/ButtonGroup.js
index c3722d255b..517eef065f 100644
--- a/packages/material-ui/src/ButtonGroup/ButtonGroup.js
+++ b/packages/material-ui/src/ButtonGroup/ButtonGroup.js
@@ -170,10 +170,12 @@ const ButtonGroupRoot = experimentalStyled(
       ...(styleProps.variant === 'outlined' &&
         styleProps.color === 'primary' && {
           borderColor: theme.palette.primary.main,
+          zIndex: 1,
         }),
       ...(styleProps.variant === 'outlined' &&
         styleProps.color === 'secondary' && {
           borderColor: theme.palette.secondary.main,
+          zIndex: 1,
         }),
       ...(styleProps.variant === 'contained' && {
         boxShadow: 'none',

@zhexuan-flux
Copy link
Author

End-users need to zoom in x2 to start being able to see the issue. I'm hesitating to add the "won't fix" flag as there is likely no other option than to remove the opacity from the border, which has other downsides, probably more important ones. So, it would be a tradeoff.

We had to solve a similar problem in #9229, but it was a lot more noticeable:

https://github.com/mui-org/material-ui/blob/cce45deec3125c1f0dd6c1c74616b5e63b027026/packages/material-ui/src/TableCell/TableCell.js#L59-L66

@oliviertassinari thanks for the triage and response! yea its not very obvious but it would be great to have them aligned. In our app, I did it by removing the negative margin-left from right button and specify border-right: 1px for the left button. MUI is setting left button's border-right transparency to 0; as a result, I can achieve the Expected Behavior I posted.

Not sure if this could help, thank you for looking into it!

@mbrookes
Copy link
Member

The Material Design specification document the outlined color as grey

You can't tell that it's opaque (what I assume you mean by grey) from that screenshot, but you can tell that it isn't from this one:

image

(Note the subtle change in color as the line crosses the streak in the background image.) The is important so that the button picks up a hint of the background color, particularly for more solid areas of color.

The current implementation was a conscious trade-off. It isn't obvious at normal magnification. Any attempted fix should ensure it doesn't affect button width and symmetry, or border transparency.

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 13, 2021

@eps1lon has worked on this in #15242. From what I recall, at that time, it wasn't obvious that Google was moving in this direction. material-components/material-components-web#5268 would support that they are (done afterward), and that 12% of opacity is what should be used.

For the given problem, I can't think of any better tradeoff than the current one. I have tried clip-path: inset(0px 1px 0px 0px); but it still leave a small rendering overlap artefact. Unless there is an objection I propose we close.

@oliviertassinari oliviertassinari added the out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) label Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ButtonGroup The React component. out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future)
Projects
None yet
Development

No branches or pull requests

3 participants