fix: support label node in compact text input#293
Conversation
✅ Heimdall Review Status
✅
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
✅
1/1
|
Denominator calculation
|
| function DatePicker() { | ||
| return <TextInput label="Pick a date" type="date" />; | ||
| } | ||
| ``` |
There was a problem hiding this comment.
We already have a DatePicker component. All the examples I removed felt unhelpful and sometimes were for the wrong platform
| paddingEnd={2} | ||
| paddingStart={start ? 0.5 : 2} | ||
| paddingTop={1} | ||
| > |
There was a problem hiding this comment.
The goal here was to have us handle the padding as best as possible so that a customer can simply use InputLabel and it all works.
However for inside label variant on web and mobile with labelNode we do need to remove vertical padding on the examples to get it to match design.
There was a problem hiding this comment.
Do you think customer would ever need to override this part when using labelNode and labelVariant === 'inside'?
<Box
background={readOnlyInputBackground}
paddingEnd={2}
paddingStart={start ? 0.5 : 2}
paddingTop={1}
>
If they want to override it, I wonder if we should expose some styles/classNames props for it? If you think the use case is not strong enough or can be overridden in other ways, I think the current way is fine.
| ref={ref} | ||
| aria-describedby={accessibilityHint} | ||
| aria-label={accessibilityLabel} | ||
| aria-labelledby={accessibilityLabelledBy} |
There was a problem hiding this comment.
Was getting this react error in dev mode
react-dom.development.js:86 Warning: React does not recognize the `accessibilityLabelledBy` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `accessibilitylabelledby` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
| {label} | ||
| </InputLabel> | ||
| )} | ||
| </Box> |
There was a problem hiding this comment.
Our mobile inputs are primarily based on set heights, but web height is more intrinsic, based on the height of the label, input, and added padding. However I found that react-native has known issues with lineHeight which prevents us from simplifying mobile facebook/react-native#39145
This is inside of NativeInput and InputStack not TextInput but mentioning since it caused me some headaches when trying to simplify our inputs.
| @@ -91,10 +91,11 @@ It's also advised you always format `helperText` with `Error: ${errorMessage}`. | |||
| <TextInput label="Label" placeholder="Placeholder" /> | |||
There was a problem hiding this comment.
Nice revamp on the examples!
| paddingEnd={2} | ||
| paddingStart={start ? 0.5 : 2} | ||
| paddingTop={1} | ||
| > |
There was a problem hiding this comment.
Do you think customer would ever need to override this part when using labelNode and labelVariant === 'inside'?
<Box
background={readOnlyInputBackground}
paddingEnd={2}
paddingStart={start ? 0.5 : 2}
paddingTop={1}
>
If they want to override it, I wonder if we should expose some styles/classNames props for it? If you think the use case is not strong enough or can be overridden in other ways, I think the current way is fine.
I could see customers wanting to customize this in the future but right now our inputs are not customizable at all really, can't even fully customize the border color let alone padding. I tried adding styles/classNames before but there was too many sub components that I think we should refactor/simplify before committing to them longterm. At least with the value being based on padding they can adjust their themevars.space to adjust the component height! |
* fix: support label node in compact text input * Cleanup inside label variant * Add examples * Cleanup docs * Undo example change * Revert mobile changes * Support labelNode in compact * Fix example * Fix inside label variant * Bump version * Update examples * Fix a11y issue * Add more tests
3 participants
What changed? Why?
This PR brings support for labelNode to TextInput for cases of labelVariant='inside' and compact.
Root cause (required for bugfixes)
We only used
labelNodein the default case and not when considering compact or labelVariant inside.UI changes
These examples weren't all on the doc site previously, but if a user tried it out this is what they would see
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