Button: Add 4 more pixels of padding to the right of icons #257
Conversation
src/Button/Button.tsx
Outdated
| @@ -519,7 +519,7 @@ export const Button = React.forwardRef<HTMLElement, Props>( | |||
| {icon && ( | |||
| <ButtonIcon | |||
| iconSize={iconSize} | |||
| className={css({ margin: iconOnly ? 0 : "0 4px 0" })} | |||
| className={css({ margin: iconOnly ? 0 : "0 8px 0 4px" })} | |||
There was a problem hiding this comment.
While we are in here, do we need the 4px on the left?
There was a problem hiding this comment.
Thanks for pointing this out, @Jephuff. I just noticed that there is a bit of a discrepancy between the icon spacing in the Space Kit design file:
...and in our current implementation in Storybook:
What makes this kind of difficult to both notice and address is that in the design file the button uses two words, but in the storybook version it uses just a single short word. So the question is what is the correct spacing?
Visually, I think it looks cleanest to have equal spacing between the icon and the left edge and the icon and the start of the word (or just a hair more between the icon and the button edge, if any).
But I also see that the buttons have a min-width applied to them, which makes this a bit tricker. Is there any reasons we need the min width on these?
Regardless of the answer to that, I'm in favor of @Jephuff's suggestion to drop the 4px on the left. 👍
There was a problem hiding this comment.
PS - case in point where this came up as awkward was in this thread for updating the new graph button, and the spacing looks weird:
There was a problem hiding this comment.
Thanks for this input! I'll axe that 4px and then send another PR testing getting rid of the min-width!
justinanastos
force-pushed
the
justin/button-icon-margin
branch
from
02736e4
to
088ed47
Compare
|
🚀 PR was released in |
📦 Published PR as canary version:
8.2.1-canary.257.6522.0✨ Test out this PR locally via:
npm install @apollo/space-kit@8.2.1-canary.257.6522.0 # or yarn add @apollo/space-kit@8.2.1-canary.257.6522.0