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

[IconButton] Add hover effect to IconButton #10871

Merged
merged 2 commits into from
Apr 3, 2018
Merged

[IconButton] Add hover effect to IconButton #10871

merged 2 commits into from
Apr 3, 2018

Conversation

SebastianSchmidt
Copy link
Contributor

@SebastianSchmidt SebastianSchmidt commented Mar 31, 2018

Closes #10869

This pull request is part of #10870.

@mbrookes mbrookes added breaking change component: icon button This is the name of the generic UI component, not the React module! labels Mar 31, 2018
@mbrookes
Copy link
Member

I'm on the fence about this change in general, but it has an unintended effect on selection controls that needs to be addressed if we're to go ahead with this PR.

@SebastianSchmidt
Copy link
Contributor Author

@mbrookes I removed the unintended hover effect from the selection controls.

@mbrookes
Copy link
Member

mbrookes commented Apr 1, 2018

Also Table pagination:

image

https://deploy-preview-10871--material-ui-next.netlify.com/demos/tables/#sorting-amp-selecting

Admittedly the right margin on the last column looks off in that example (as does the column spacing in the Table component in general), but I'm not sure whether the pagination icons should have the hover effect. (I'm still adjusting to it 😄@oliviertassinari, thoughts?

@leMaik
Copy link
Member

leMaik commented Apr 1, 2018

@mbrookes The right margin of the buttons looks a bit off in that demo, because that table has >100% width and is horizontally scrollable (PR that fixes that).

If you scroll it to the right, everything is fine (and the margin is carefully crafted to match the spec's pictures of the table pagination). I think the hover effect would be great on those buttons, so that the user can see that they are clickable. 👍

@mbrookes
Copy link
Member

mbrookes commented Apr 1, 2018

that table has >100% width and is horizontally scrollable

Oh, so it is. We need to fix the button overflow elsewhere then.

@oliviertassinari oliviertassinari self-assigned this Apr 2, 2018
@oliviertassinari oliviertassinari added new feature New feature or request and removed breaking change labels Apr 3, 2018
@oliviertassinari oliviertassinari merged commit 78f7e10 into mui:v1-beta Apr 3, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 3, 2018

@SebastianSchmidt I have changed the fade value to stick to the material-web-components value. https://github.com/material-components/material-components-web/blob/06e39b18e1ecc56785ea0cf82ba093c17fde516f/packages/mdc-ripple/_variables.scss#L23. However, I start to believe that we should revert this PR, and move the logic to the ButtonBase. What do you think about it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: icon 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.

4 participants