Add icon-only button support#789
Conversation
| justify-content: space-between; | ||
| align-items: center; | ||
| padding: 15px; | ||
| padding: 12px 15px; |
There was a problem hiding this comment.
Are we moving away from using the variables in mashlib light theme? There are spacing variables for 12px and 15px. Also, wondering why we don't use rem? Curious for my work now on source-pane to know whether I should be doing things differently.
There was a problem hiding this comment.
Ideally yes, we should be using the variables from the design system css files (or primitive css files, which will probably be merged in the end).
| color: var(--solid-ui-color-gray-600); | ||
|
|
||
| icon-lucide-x { | ||
| width: 22px; |
There was a problem hiding this comment.
wondering about the size? I need for instance a 16px icon for the source-pane header.
There was a problem hiding this comment.
The way icons are configured right now, you need to set their width/height like this yes. But I'd try to use variables whenever possible, instead of hard-coded px.
NoelDeMartin
left a comment
NoelDeMartin
left a comment
There was a problem hiding this comment.
I have taken a look at the PR, and I think it's not necessary because the current code already supports what this is trying to achieve.
Here's some details:
- I already created an "icon only" configuration for buttons, which is basically using the "ghost" variant and including an
iconslot. This is also showcased in the storybook with the Ghost variant, but this PR breaks it (please make sure to always check the storybook before changing any UI code). - We could create an iconOnly variant or attribute in the Button component... But that's a matter of whether we like it more or less than the current component API. I'm not against it if you have suggestions for a better API, but I don't think it should change the implementation (unless we find some reason to).
- The iconOnly implementation from this PR creates a button that is 42x42px. That goes against the current design, which produces a 35x35px button (the one we currently have is 34x34px though, that should be fine for now). However, I know that 35x35 is too small for a clickable target... That's why the actual clickable area is 42x42px, configured with the
--solid-ui-clickable-areavariable. - The PR also changes the close button from the modal, but again I think this was already fine in the current implementation (which is using the ghost variant + icon slot).
Did I miss something?
4 participants
Summary