fix(icon-button): hover state fix, and remove HOC #871
Conversation
| @@ -169,6 +169,7 @@ const getThemeStyles = theme => { | |||
| switch (theme) { | |||
| case 'green': | |||
| return css` | |||
| &:hover, | |||
| &:active { | |||
There was a problem hiding this comment.
this was missing hover, so you never saw green on hover
montezume
force-pushed
the
ml-try-icon-inline-flex
branch
from
3b4e2bf to
f4536ac
Compare
| @@ -73,21 +68,16 @@ export const IconButton = props => { | |||
| css={css` | |||
| width: 100%; | |||
| height: 100%; | |||
| display: flex; | |||
There was a problem hiding this comment.
my hope is this fixes @adnasa's issue
| <IconButton | ||
| icon={<InformationIcon />} | ||
| label="A label text" | ||
| onClick={() => {}} | ||
| isToggleButton={true} | ||
| isToggled={true} |
There was a problem hiding this comment.
these tests make more sense now because the output will actually be blue...
| @@ -42,10 +40,6 @@ export const IconButton = props => { | |||
| const isActive = props.isToggleButton && props.isToggled; | |||
| return ( | |||
| <div | |||
There was a problem hiding this comment.
Is it also possible to get rid of this div and apply the styles to the AccessibleButton component?
There was a problem hiding this comment.
giving it a shot now
There was a problem hiding this comment.
okay removed it, and VRTs passed 🤷♂
There was a problem hiding this comment.
Nice!
I always found it weird that we had an AccessibleButton component which already deals with the mouse events, but then wrapped within a dull div which then reimplemented those mouse events with the HoC... I guess there must have been some other reason before, but something that got refactored? 🤔
It makes sense that it can be removed given how useless it seemed at the moment 🤷♂
There was a problem hiding this comment.
do you want to approve then 🧐
| @@ -169,7 +169,7 @@ const getThemeStyles = theme => { | |||
| switch (theme) { | |||
| case 'green': | |||
| return css` | |||
| &:active { | |||
| "bundled": 866961, | ||
| "minified": 596248, | ||
| "gzipped": 121235, | ||
| "bundled": 879277, |
There was a problem hiding this comment.
the problem is we don't run "yarn build" enough so this is always outdated.
Summary
Fixes a bug with hovering IconButtons with "green" theme. Also, refactors the component to remove the usage of the HOC.
Before
After