-
Notifications
You must be signed in to change notification settings - Fork 99
feat(textarea): new SHINE styles and Svelte component #2077
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…into sal/SPARK-68
This reverts commit 5850d2a.
* Various input sizing, layout, docs tweaks * Fix sm, lg w-axis padding
🦋 Changeset detectedLatest commit: f49e103 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 |
dancormier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a handful of suggestions but nothing too significant. Overall, we're looking good. Once my comments are addressed* and "new": true is added for textarea in navigation.json, this should be good to go.
@mukunku Thanks for the nice work and for that :has(> .s-textarea) trick I look forward to stealing at a later date 🙂
* one comment could use some design input. I'm fine with it being handled in a follow up if design needs a moment to address.
packages/stacks-classic/lib/components/input_textarea/input_textarea.less
Show resolved
Hide resolved
packages/stacks-classic/lib/components/input_textarea/input_textarea.less
Outdated
Show resolved
Hide resolved
| --_in-min-h: calc(var(--su32) + var(--su4)); // 36px | ||
|
|
||
| &&__sm { | ||
| --_in-min-h: calc(var(--su32) - var(--su4)); // 28px | ||
| } | ||
|
|
||
| &&__lg { | ||
| --_in-min-h: calc(var(--su48) - var(--su4)); // 44px |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up from previous comment
These values will need to be updated to match the Figma values.
sm: 55px- default: 69px
lg: 96px
| --_in-min-h: calc(var(--su32) + var(--su4)); // 36px | |
| &&__sm { | |
| --_in-min-h: calc(var(--su32) - var(--su4)); // 28px | |
| } | |
| &&__lg { | |
| --_in-min-h: calc(var(--su48) - var(--su4)); // 44px | |
| --_in-min-h: calc(var(--su64) - var(--su4) + var(--su1)); // 69px | |
| &&__sm { | |
| --_in-min-h: calc(var(--su48) + var(--su6) + var(--su1)); // 55px | |
| } | |
| &&__lg { | |
| --_in-min-h: var(--su96); | |
| } |
@CGuindon @mukunku I think we should consider some different values here. It seems like the target is moreso getting three lines of text within the textarea without introducing a scrollbar. If that's correct, here are values that might achieve what we're looking for:
sm:58px- default:
72px lg:104px
Note
I have not tested this outside of my Mac, so these values might need to be tweaked for the same scroll-less three-liner textareas on other OSes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intention was to make the min-height as tall as a single line just so the user can't make the input very small and mess up the icon (since textarea is user resizable):

Do we really want to restrict developers/users from setting a smaller height than minimum 3 lines? Maybe they'd want the input to start small and expand as needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to restrict developers/users from setting a smaller height than minimum 3 lines? Maybe they'd want the input to start small and expand as needed?
I think we want to make the right/consistent thing the easy thing while providing escape hatches that create a little friction but still allow for flexibility. Since the design shows these sizes, I'd expect to make these sizes the easy thing. A developer still could set whatever height/max-height they'd need by overriding the max-height by adding a .hmx-* class. They'd have to hop into the inspector to understand that there's a max-height set, but it's doable. Most of our atomic classes include !important, so they'll always take precedence over the component styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering this a bit more, I think it could make sense to set the min-height the way you did @mukunku and then setting a height for each size of textarea.
This way, a user could only resize it down to a single line and not beyond while having the initial size reflect the designs in the Figma. Everything could still be overridden by a dev but the defaults would be sensible and consistent. Does this seem reasonable @mukunku?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great compromise and I think would work great! Made the change now: f49e103
I went with the following sizes which are almost the same as the Figma's:
- sm: 54px
- default: 70px
- lg: 96px
I can adjust to your recommendation as well if we think that's better. Let me know what you think @dancormier 🙏🏾
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to merge as-is but happy to change as a follow-up 👍🏾
dancormier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is in a good place IMO. I've responded to your comments @mukunku and a couple things might need very minor tweaking but nothing that needs to hold up this PR. I'm happy to leave any further changes up to your discretion.
Nice work here ❤️




Summary
This PR makes some small tweaks to update the textarea component to match the new designs. It builds on the new input designs in #2070 so this should be merged after that one.
Additionally, this PR exposes a new
TextAreaSvelte component. This is a clone of the TextInput component minus the Input component specific bits.Breaking changes
Following sizes have been removed:
s-textarea__mds-textarea__xlHow to Test