refactor: use theme spacing for Select min height#317
refactor: use theme spacing for Select min height#317haoruikun-cb wants to merge 3 commits intomasterfrom
Conversation
🟡 Heimdall Review Status
🟡
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
🟡
0/1
|
Denominator calculation
|
haoruikun-cb
temporarily deployed
to
production
GitHub Actions
Inactive
haoruikun-cb
temporarily deployed
to
production
GitHub Actions
Inactive
| const LABEL_VARIANT_INSIDE_HEIGHT = 32; | ||
| const COMPACT_HEIGHT = 40; | ||
| const DEFAULT_HEIGHT = 56; | ||
| const LABEL_VARIANT_INSIDE_HEIGHT = 'var(--space-4)'; |
There was a problem hiding this comment.
@haoruikun-cb won't the values be string instead of number now?
There was a problem hiding this comment.
This is fine since HStack minHeight props takes in either number or string
| const DEFAULT_HEIGHT = 56; | ||
| const LABEL_VARIANT_INSIDE_HEIGHT = 'var(--space-4)'; | ||
| const COMPACT_HEIGHT = 'var(--space-5)'; | ||
| const DEFAULT_HEIGHT = 'var(--space-7)'; |
There was a problem hiding this comment.
I'm nervous for how this approach would act in cases where spacing is increased.
If their unit spacing = 12px but their fonts stay the same the input would be too large now.
It still could very well likely be an improvement over the current approach, I'm not sure how the current min height code would act.
There was a problem hiding this comment.
Having larger spacing works. They can also have larger font size for theme. It will be up to the customer what config they want. I added a larger Select theme example and ut
There was a problem hiding this comment.
This example with spacing is smaller (1 space = 4px) our min height is 28px but we get 36px. So even though our min height doesn't match it is still the correct size.
When we have larger spacing (1 space = 16px) our min height is 112px but yet it should be 72 based on this inner Select sizing.
So we are off however the output isn't too bad I suppose
When comparing this with TextInput though, they only align correctly at the default spacing (and when labelVariant does not equal inside hmm)
There was a problem hiding this comment.
I believe we should get rid of minHeight and focus on the intrinsic size of the component, even setting min height to 0 didn't work in my tests, the padding seemed too small for default case.
There was a problem hiding this comment.
Yeah you are right. Let me investigate how we can refactor this to fix the intrinsic height.
haoruikun-cb
temporarily deployed
to
production
GitHub Actions
Inactive
4 participants
What changed? Why?
Refactor Select to use theme token for min height, so it can react to ThemeProvider changes like TextInput.
Root cause (required for bugfixes)
UI changes
Testing
How has it been tested?
Testing instructions
Illustrations/Icons Checklist
Required if this PR changes files under
packages/illustrations/**orpackages/icons/**Change management
type=routine
risk=low
impact=sev5
automerge=false