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

[Button] Use fixed outlined stroke color #15242

Closed
wants to merge 2 commits into from

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Apr 7, 2019

Closes #9526

Breaking

Outlined buttons use a fixed border color for the containers independent of color variant or state.

Get the old look

import React from 'react';
import { fade, makeStyles } from '@material-ui/core/styles';
import Button from '@material-ui/core/Button';

const useOldStyles = makeStyles(theme => {
  return {
    outlined: {
      '&$disabled': {
        border: `1px solid ${theme.palette.action.disabled}`,
      },
    },
    outlinedPrimary: {
      border: `1px solid ${fade(theme.palette.primary.main, 0.5)}`,
      '&:hover': {
        border: `1px solid ${theme.palette.primary.main}`,
      },
    },
    outlinedSecondary: {
      border: `1px solid ${fade(theme.palette.secondary.main, 0.5)}`,
      '&:hover': {
        border: `1px solid ${theme.palette.secondary.main}`,
      },
    },
  };
});

function OldButton(props) {
  const classes = useOldStyles(props);
  return <Button {...props} classes={classes} />;
}

Visual diff

docs: next vs. docs: PR

argos: next vs. argos: PR

@eps1lon eps1lon added design: material This is about Material Design, please involve a visual or UX designer in the process component: button This is the name of the generic UI component, not the React module! labels Apr 7, 2019
@eps1lon eps1lon added this to the v4 milestone Apr 7, 2019
It put to much emphasis on the button
@mui-pr-bot
Copy link

@material-ui/core: parsed: -0.11% 😍, gzip: -0.07% 😍

Details of bundle changes.

Comparing: 8a67ecc...efb6af5

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.11% -0.07% 347,956 347,578 89,782 89,718
@material-ui/core/Paper 0.00% 0.00% 68,404 68,404 20,053 20,053
@material-ui/core/Paper.esm 0.00% +0.01% 🔺 60,735 60,735 18,785 18,787
@material-ui/core/Popper 0.00% 0.00% 34,930 34,930 11,919 11,919
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 17,877 17,877 5,962 5,962
@material-ui/core/useMediaQuery 0.00% 0.00% 2,463 2,463 1,044 1,044
@material-ui/lab 0.00% +0.01% 🔺 148,083 148,083 43,917 43,921
@material-ui/styles 0.00% 0.00% 53,143 53,143 15,444 15,444
@material-ui/system 0.00% 0.00% 17,132 17,132 4,519 4,519
Button -0.43% -0.17% 88,669 88,291 26,420 26,374
Modal 0.00% +0.02% 🔺 82,688 82,688 24,802 24,806
colorManipulator 0.00% 0.00% 3,745 3,745 1,537 1,537
docs.landing 0.00% 0.00% 49,820 49,820 10,807 10,807
docs.main -0.06% -0.07% 644,555 644,179 200,662 200,525
packages/material-ui/build/umd/material-ui.production.min.js -0.12% -0.05% 296,610 296,252 82,782 82,744

Generated by 🚫 dangerJS against efb6af5

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 7, 2019

Closes #9526

I was interested in changing the ripple behavior. We were aware of this difference in the outline color with the specification when implementing the outlined variant. We should be able to find some old discussion on why we didn't follow the specification.

@eps1lon
Copy link
Member Author

eps1lon commented Apr 7, 2019

I was interested in changing the ripple behavior.

In what capacity?

@eps1lon eps1lon marked this pull request as ready for review April 7, 2019 17:51
@mbrookes
Copy link
Member

mbrookes commented Apr 8, 2019

We should be able to find some old discussion on why we didn't follow the specification.

I recall this being discussed - there may even have been a previous PR. Rightly or wrongly, we agreed that deviating from the spec on this occasion was the better choice.

@eps1lon
Copy link
Member Author

eps1lon commented Apr 8, 2019

@mbrookes Would be helpful to reference this discussion. The old aggrement might be outdated since the other components/spec changed.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 8, 2019

The old aggrement might be outdated since the other components/spec changed.

@eps1lon The specification didn't change as you can see it in #11346. Our implementation was further changed in #12473.

In what capacity?

I believe the ripple of the specification has a larger initial size, at least in the list item case 🤔.

@eps1lon
Copy link
Member Author

eps1lon commented Apr 8, 2019

I don't see any arguments in #12473 for not following the spec. The added classes are still available after this change.

Since then angular has switched to a fixed border color.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 8, 2019

I have looked at the other UI libraries (Bootstrap, Bulma, Semantic UI, Basscss, Tailwind). Using the same color for the border than the text seems to be a common convention. I could only find one usage of the outlined button on a google product: Google docs. They change the border color when hovered (against the spec) (it's grey otherwise).

This dilemma is perfect for a Twitter poll :).

@mbrookes
Copy link
Member

mbrookes commented Apr 8, 2019

@eps1lon Apologies, I couldn't find it last night / this morning (or whatever ungodly time it was!) I was probably misremembering the PR @oliviertassinari linked to add color to the border, rather than one to remove it. You're right, there isn't a discussion per-se, just an implicit acceptance that it represented an improvement over the spec.

I'm acutely aware this contradicts what we normally say about spec > MCW, but I think they got it right this time (the color, not the width!). That said, I'm not invested enough in it to have a heated debate about it. 😜

@eps1lon
Copy link
Member Author

eps1lon commented Apr 8, 2019

This dilemma is perfect for a Twitter poll :).

I would have a problem with that. Which spec points are we polling and which not?

I'm fine with the decision to follow other reference implementations instead of the spec since we had it for so long and nobody ever raised this issue.

We could collect these decision into a single theme and make it public i.e. a "strict" theme and a "loose" theme that is more opinionated. I think this would help with a lot of discussions we have.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 8, 2019

I would have a problem with that. Which spec points are we polling and which not?

We have made a call 8 months ago to accept a change that goes against the specification in order to be closer to what the other libraries are doing (a.). The specification on this part didn't change over that period of time. And yet, we had zero requests from people to change the behavior. Isn't that weird? It's not like the Button is our most used component (b.) What's a Material Design specification good for if neither Google products nor its reference implementation follows it? (c.)

We have 3 signals (a, b, c) that we made the right call by not following the specification. I would vote for not changing it. But I'm happy to be proven otherwise, hence the poll suggestion.

@oliviertassinari
Copy link
Member

We could collect these decision into a single theme and make it public i.e. a "strict" theme and a "loose" theme that is more opinionated. I think this would help with a lot of discussions we have.

Interesting.

@eps1lon
Copy link
Member Author

eps1lon commented Apr 9, 2019

I would have a problem with that. Which spec points are we polling and which not?

[...]

Seems like you misunderstood my point. I wasn't arguing for or against this particular change. You didn't provide any argument other than the absent of an argument. My main point was that we shouldn't randomly decide what spec points are decided by twitter! poll that provides no context. How long should this be considered the standard? How many votes make it legitimate? Who should vote etc.

You can't seriously argue that the absence of a voice against X is argument enough for X. That's just naive and irresponsible.

So I will close this (and the issue since it is incomplete) and flesh out the strict vs. loose theme idea.

@eps1lon eps1lon closed this Apr 9, 2019
@eps1lon eps1lon deleted the feat/Button/spec-outlined branch April 9, 2019 19:11
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 9, 2019

I have found another case for an outlined button on a Google product, this time it's Chrome.

default
Capture d’écran 2019-04-09 à 16 50 10

hover
Capture d’écran 2019-04-09 à 16 50 16

we shouldn't randomly decide what spec points are decided by twitter!

We are building Material-UI for our users, we are accountable to them. At the end of the day, they decide, I don't. Our job is to build intuition on what resonates with them. The simplest binary choice a using Material-UI vs not using it weights a lot compounded. A Twitter poll is one of the best imperfect tools we have to shorten the feedback loop. We can provide a bit of context in a poll.

You can't seriously argue that the absence of a voice against X is argument enough for X. That's just naive and irresponsible.

I'm sure there is some literature about this: decision making by the least resistance. I would use a different angle to see the situation. If nobody mentions it: maybe it's not important?

@eps1lon
Copy link
Member Author

eps1lon commented Apr 9, 2019

I'm sure there is some literature about this: decision making by the least resistance.

All ears. Show me some literature that shows that the best development happens by Yes/No questions answered by random persons on the internet.

@oliviertassinari
Copy link
Member

@eps1lon 😆. It remembers me a previous discussion. How do you handle a situation where you have a safe option (follow the specification is our case) and another that might yield more user happiness but is contestable (not following the specification, match the other UI frameworks)? My answer: you test and you learn 🔬. Start with the option that can yield the best ROI. If you don't receive push-backs, keep it.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 11, 2019

I will keep track of the occurrences of the outlined version I can find here.

Capture d’écran 2019-04-11 à 14 11 54

@eps1lon eps1lon restored the feat/Button/spec-outlined branch June 8, 2019 10:20
@oliviertassinari
Copy link
Member

Google seems to have pushed forward in the direction of the changes we are originally here: material-components/material-components-web#5268

@eps1lon eps1lon deleted the feat/Button/spec-outlined branch September 14, 2021 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: button This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Button] Update the implementation to match the specification when outlined
4 participants