fix: make icons inside IconButtons flex to fit button container #154
Conversation
|
it looks like that focus ring issue already exists: https://cauldron.netlify.app/components/IconButton but I agree we should fix it |
??? This is what I see on https://cauldron.netlify.app/components/IconButton: There's a 1px spacing around the button icon, which doesn't exist in this PR. For comparison:
|
|
I've updated the indicators to reflect Aaron's designs by taking from what we were using for the other buttons. Updated icons are in the main description. |
There was a problem hiding this comment.
One more small minor change: Add pointer-events: none to the .IconButton:before pseudo element. We don't want the pseudo element to be triggering click events or activating when it's not supposed to. pointer-events: none should prevent both of these behaviors.
packages/styles/icon-button.css
Outdated
| background-color: var(--icon-button-background-color); | ||
| color: var(--icon-button-icon-color); | ||
| margin: 2px; | ||
| padding: 0; | ||
| --button-hover-outline-color: var(--top-bar-text-color); |
There was a problem hiding this comment.
This looks like a typo. I don't think --button-hover-outline-color is a valid CSS property.
There was a problem hiding this comment.
Wasn't a typo, it's a custom key that we were using for the hover outline. I essentially stole this methodology from the vanilla button css. Since it isn't repeated, I'll remove it for now.
packages/styles/icon-button.css
Outdated
| .IconButton svg { | ||
| height: calc(var(--button-thin-height) - 4px); | ||
| width: calc(var(--button-thin-height) - 4px); | ||
| .IconButton div { |
There was a problem hiding this comment.
One more small thing I noticed, do you think this should be more specific? i.e. .IconButton .Icon?
div seems a bit too generic.
New hover indicator:
New focus indicator:
These indicators were taken from what we were using for the vanilla buttons and were tested to work with stretched buttons.