-
Notifications
You must be signed in to change notification settings - Fork 107
feat(ui-number-input): rewrite NumberInput to the new theming system #2271
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
base: v12
Are you sure you want to change the base?
Conversation
| transition: 'outline-color 0.2s, outline-offset 0.25s' | ||
| }), | ||
| outlineOffset: '-0.8rem', | ||
| outlineStyle: 'solid', |
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 is a small fix for the focus outline not showing because CSS styles were reset by FormFieldLayout
| * Not visible when `disabled` or `readonly` | ||
| */ | ||
| placeholder?: string | ||
|
|
||
| /** | ||
| * Whether or not the text input is required. | ||
| */ | ||
| isRequired?: boolean | ||
|
|
||
| /** | ||
| * Whether or not to display the up/down arrow buttons. | ||
| * They are not visible when `readonly` |
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.
these are visual changes requested by design
7d22854 to
82c3075
Compare
adamlobler
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.
Looking good, I just left two minor comments related to the arrow icon colors. Other than that, the labels and message colors/sizes seem a bit strange, but I assume that will be handled in the formField component.
| {customIcons?.increase ? ( | ||
| callRenderProp(customIcons.increase) | ||
| ) : ( | ||
| <IconArrowOpenUpLine /> |
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.
We should define the icon colors here for each state, now it stays black in every theme and state.
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've fixed this, good catch
| {customIcons?.decrease ? ( | ||
| callRenderProp(customIcons.decrease) | ||
| ) : ( | ||
| <IconArrowOpenDownLine /> |
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.
We should define the icon colors here for each state, now it stays black in every theme and state.
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've fixed this, good catch
19d6593 to
e85ac1b
Compare
| const [upButtonState, setUpButtonState] = useState<ArrowButtonColors>( | ||
| 'actionSecondaryBaseColor' | ||
| ) | ||
| const [downButtonState, setDownButtonState] = useState<ArrowButtonColors>( | ||
| 'actionSecondaryBaseColor' |
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 is quite an ugly way to handle the buttons state, but the original code had this as <button> and I did not want to change it. Its needed because the new design adds hover/down states to the icons
d783b68 to
67c31f8
Compare
|
| ), | ||
| '@instructure/ui-grid$': path.resolve(import.meta.dirname, '../ui-grid/src/'), | ||
| '@instructure/ui-i18n$': path.resolve(import.meta.dirname, '../ui-i18n/src/'), | ||
| '@instructure/ui-icons-lucide': path.resolve(import.meta.dirname, '../ui-icons-lucide/src/'), |
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 fix recompiles + reloads docs when one changes something in this package
| return ( | ||
| <span css={styles?.lucideIcon} className={className} style={style}> | ||
| <Icon | ||
| name={Icon.displayName} |
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.
some unit tests target these by the display name
adamlobler
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.
Thanks for the modification to the arrows, it looks great! 🙌 I found 2 minor things in the showcase, but other than that its okay to merge from my side.
| <NumberInput | ||
| renderLabel='has error' | ||
| placeholder="placeholder" | ||
| messages={[{ text: 'This is an error.', type: 'error' }]} |
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’m not sure about this, but as far as I know, this error message is the legacy format that users shouldn’t use. If that’s true, then we should show the newError message here instead.
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 is part of FormField and not this component, we will fix it there. (and AFAIK type: 'error will be the new error, and we'lll remove the old type error)
| <NumberInput renderLabel="Default-size input" /><br/> | ||
| <NumberInput size="large" renderLabel="Large-size input" /> | ||
| </div> | ||
| <Flex gap='2rem' direction='column'> |
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.
The "gap" doesn't apply the raw value here for some reason, we should use gap='medium' instead.
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.
good catch, fixed it
67c31f8 to
9c7066c
Compare
joyenjoyer
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.
gj
741fcb3 to
92082f5
Compare
- Converted `NumberInput` from class-based to functional component - It uses the theme from TextInput, they are the same in Figma - The styling of the upper label/messages are not done, they are coming in `FormFieldLayout` Tokens are also not fully used (they are used only in `TextInput`): - `fontSizeSm`, `heightSm`, `paddingHorizontalSm`: This component has no small size - `gapContent`: This is a gap between the text and elements rendered after it, `NumberInput` does not have such To test: - Check the examples in the docs, they should function exactly as before - Compare its CSS to the ones in Figma Completes INSTUI-4814
92082f5 to
fa66e5a
Compare
Convert NumberInput to the new theming system. Notes:
TextInput, they are the same in FigmaFormFieldLayoutTokens are also not fully used (because they are used only in
TextInput):fontSizeSm,heightSm,paddingHorizontalSm: This component has nosmallsizegapContent: This is a gap between the text and elements rendered after it,NumberInputdoes not have suchTo test: