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] Make the outlined variant better leverage the color #12473

Merged
merged 3 commits into from
Aug 12, 2018

Conversation

essuraj
Copy link
Contributor

@essuraj essuraj commented Aug 11, 2018

  • Changed the color of the outline of the button when the button color is primary and secondary based on the theme used.
  • Changed the outline width to 2px from 1px as it is what the official material docs use, plus they look good.

image

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 11, 2018

Changed the color of the outline of the button when the button color is primary and secondary based on the theme used.

I have no objection to this change. It's what vuetify and angular material are doing. It's also creating more override flexibility with the two new class keys 👍 .

Changed the outline width to 2px from 1px as it is what the official material docs use, plus they look good.

MCW is the only one to do this, it even goes against the official specification. I don't think that we should be following them.

@mbrookes What do you think?

@oliviertassinari oliviertassinari changed the title theme-aware-outlined-button [Button] Make the outlined variant better leverage the color Aug 11, 2018
@oliviertassinari oliviertassinari added new feature New feature or request component: button This is the name of the generic UI component, not the React module! labels Aug 11, 2018
@mbrookes
Copy link
Member

mbrookes commented Aug 11, 2018

I agree with @oliviertassinari about not changing the border-width.

Regarding the border color, perhaps it might be better if only applied when focused, in order to stay closer to the spec?

kapture 2018-08-11 at 17 14 28

Alternatively, perhaps it could take the primary /secondary color, but with reduced opacity when not hovered or focused?

kapture 2018-08-11 at 17 51 12

@essuraj
Copy link
Contributor Author

essuraj commented Aug 12, 2018

changed back to 1px width and added color opacity on hover

@oliviertassinari oliviertassinari self-assigned this Aug 12, 2018
@mbrookes
Copy link
Member

border needs to be added to the transition, and you'll need to update the docs with yarn docs:api, fix the failing test, and ideally add tests for these styles.

What should the style be when focused?

@essuraj
Copy link
Contributor Author

essuraj commented Aug 12, 2018

2018-08-12_19-29-50
right now it looks like so, this is my first pull request, i dont understand where i must change so that the tests pass. Running yarn docs:api is building all md files and shows i must commit 200 files..

i'm getting this

1608 passing (5s)
1 failing

  1. server side should be able to extract the styles:

    AssertionError: expected 'MuiTouchRipple-root-30' to equal 'MuiTouchRipple-root-28'

    • expected - actual

    -MuiTouchRipple-root-30
    +MuiTouchRipple-root-28

    at Context. (packages\material-ui\src\styles\MuiThemeProvider.test.js:103:20)

@oliviertassinari oliviertassinari force-pushed the theme-aware-outlined-button branch from 7edea1d to a43d2cf Compare August 12, 2018 15:13
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbrookes The pull request should be green now. I haven't changed the direction @essuraj took. Have a final look at it :).

@mbrookes mbrookes merged commit 4a61acd into mui:master Aug 12, 2018
@mbrookes
Copy link
Member

@essuraj Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: button This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants