feat(label): update styles to match the new SHINE specs #2163
Conversation
🦋 Changeset detectedLatest commit: 49e70d6 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for stacks-svelte ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| // While I'd prefer to enforce the requirement of the parent class, it's too much of a lift at this moment. | ||
| // We'll come back to it, hopefully when we have a pill component to replace the current usage of `.s-label--status` | ||
| // without the base label class. | ||
| // Helper mixin to apply styles to elements |
There was a problem hiding this comment.
@dancormier I found it pretty difficult to add the new --su8 gap between label and inputs. This is because the label component is an entirely separate element. I ended up creating this mixin to avoid some duplication. It goes off and apply the padding if it finds a s-input s-textarea or s-select as a sibling (or nested sibling). Any thoughts on how we could make this better?
There is always the option of leaving this up to the consumers (they add themselves the appropriate atomic classes) but I feel it defeats the purpose of centralizing this component for consistency across pages.
There was a problem hiding this comment.
Ah, yeah this seems tricky.
My expectations would be that components like this don't handle the spacing themselves if possible.
I think the optimal (but possibly more effortful) approach would be to leverage .s-form-group in as a wrapper for label + any other input/textarea/description/etc. This would allow for tighter control and would lift any contextual styles up out of the component where they might result in confusing spacing for the consumer.
I whipped up this PR to demonstrate what I think might work for us in this regard. Let me know what you think about that approach.
CGuindon
left a comment
CGuindon
left a comment
There was a problem hiding this comment.
- I noticed you have 8px padding below the
labelbut there's also a 4px gutter spacing. If you need to keep the gutter you could do 4+4 so that the total spacing is only 8.
- On the small size, reduce the gutter between the label and input to 2 (instead of 4) and it will make 6px total with the 4px padding that's already there.
- Large has a total of 10px right now (8px gutter + 2px padding) – that's good, leave as is.
- Similarly with the description line, there's both a 4px padding and 4px gutter space doubling the amount (should be 4px total).
- For the Status variants, the badge component isn't perfectly aligned vertically in the middle. It looks like the it's aligned with the baseline of the text but because the badge font size is smaller, it aligns everything else lower.
- There's a "space" added after each label title before the badge in the docs. Makes it seem like there's too much padding, we can delete this character.
dancormier
left a comment
dancormier
left a comment
There was a problem hiding this comment.
@giamir thanks for tackling the label. Everything besides spacing looks good to me. I left a comment on your question with a link to a PR that I think might be helpful. Feel free to take what is helpful and leave the rest. And let me know if you want to discuss this or pair up.
SPARK-69
Figma
This PR:
s-label__md,s-label__xls-label--status,s-label--status__required,s-label--status__new,s-label--status__betain favor of using the news-badgestates instead.Label.sveltecomponent accordingly and the components using it internally (TextInput, TextArea and Select)I took the liberty of reducing the padding bottom between label and input for
smvariant from--su8to--su4even if in Figma there was no explicit guidance regarding sizes and related paddings.Label Docs
Storybook (TextInput, TextArea, Select)