Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: v12
Choose a base branch
from
+483 −357
Open

feat(ui-number-input): rewrite NumberInput to the new theming system #2271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,5 +131,5 @@
"browserslist": [
"extends @instructure/browserslist-config-instui"
],
"packageManager": "pnpm@10.23.0"
"packageManager": "pnpm@10.23.0+sha512.21c4e5698002ade97e4efe8b8b4a89a8de3c85a37919f957e7a0f30f38fbc5bbdd05980ffe29179b2fb6e6e691242e098d945d1601772cad0fef5fb6411e2a4b"
}
1 change: 1 addition & 0 deletions packages/__docs__/resolve.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ const alias = {
),
'@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/'),
Copy link
Collaborator Author

@matyasf matyasf Nov 28, 2025

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

'@instructure/ui-img$': path.resolve(import.meta.dirname, '../ui-img/src/'),
'@instructure/ui-instructure$': path.resolve(import.meta.dirname, '../ui-instructure/src/'),
'@instructure/ui-link$': path.resolve(import.meta.dirname, '../ui-link/src/'),
Expand Down
1 change: 1 addition & 0 deletions packages/emotion/src/styleUtils/calcFocusOutlineStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ const calcFocusOutlineStyles = (
transition: 'outline-color 0.2s, outline-offset 0.25s'
}),
outlineOffset: '-0.8rem',
outlineStyle: 'solid',
Copy link
Collaborator Author

@matyasf matyasf Nov 27, 2025

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

outlineColor: alpha(outlineStyle.outlineColor, 0),
'&:focus': {
...outlineStyle,
Expand Down
3 changes: 2 additions & 1 deletion packages/emotion/src/useStyle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ const isNewThemeObject = (obj: BaseThemeOrOverride): obj is Theme => {
const useStyle = <P extends GenerateStyleParams>(useStyleParams: {
generateStyle: P
params?: SecondParameter<P>
componentId: keyof NewComponentTypes
// needs to be a string too because it might be a child component
componentId: keyof NewComponentTypes | string
themeOverride: ThemeOverrideValue | undefined
displayName?: string
//in case of a child component needed to use it's parent's tokens, provide parent's name
Expand Down
1 change: 1 addition & 0 deletions packages/ui-icons-lucide/src/wrapLucideIcon/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ export function wrapLucideIcon(Icon: LucideIcon): LucideIcon {
return (
<span css={styles?.lucideIcon} className={className} style={style}>
<Icon
name={Icon.displayName}
Copy link
Collaborator Author

@matyasf matyasf Nov 28, 2025

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

ref={handleElementRef}
size={numericSize}
color={customColor}
Expand Down
8 changes: 2 additions & 6 deletions packages/ui-icons-lucide/src/wrapLucideIcon/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,16 +49,12 @@ const generateStyle = (
if (color === 'inherit') {
colorStyle = { color: 'inherit' }
} else if (color in componentTheme) {
/**
* Direct theme property access
*/
// Direct theme property access
colorStyle = {
color: componentTheme[color as keyof typeof componentTheme] as string
}
} else {
/**
* Custom CSS color (e.g., "#ff0000", "rgb(255, 0, 0)")
*/
// Custom CSS color (e.g., "#ff0000", "rgb(255, 0, 0)")
colorStyle = { color }
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/ui-number-input/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
"@instructure/shared-types": "workspace:*",
"@instructure/ui-a11y-utils": "workspace:*",
"@instructure/ui-form-field": "workspace:*",
"@instructure/ui-icons": "workspace:*",
"@instructure/ui-icons-lucide": "workspace:*",
"@instructure/ui-react-utils": "workspace:*",
"@instructure/ui-utils": "workspace:*",
"@instructure/uid": "workspace:*",
Expand Down
43 changes: 34 additions & 9 deletions packages/ui-number-input/src/NumberInput/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ Note that this field **does not work
uncontrolled** - you must pass event handlers if you want it to respond to
user input.

This example handles arrow buttons, up/down arrow keys, and typing into
the input. It also includes an `onBlur` handler that displays an error message
if the input is invalid or missing.
This example handles arrow buttons, up/down arrow keys, and typing into the input. It also includes an `onBlur` handler that displays an error message if the input is invalid or missing.

```js
---
Expand Down Expand Up @@ -150,16 +148,43 @@ render(<Example />)

> Note: `NumberInput` accepts a string or number as its `value`. However, the value returned by the `onChange` callback is always a string and should be converted to a number before attempting to augment it.

NumberInput comes in 2 sizes. The default size is "medium".
You can see here most of the visual states of the component.

```js
---
type: example
type: example
---
<div>
<NumberInput renderLabel="Default-size input" /><br/>
<NumberInput size="large" renderLabel="Large-size input" />
</div>
<Flex gap='medium' direction='column'>
<NumberInput
renderLabel='normal'
placeholder="placeholder"
/>
<NumberInput
interaction='disabled'
renderLabel='disabled'
placeholder="placeholder"
/>
<NumberInput
interaction='readonly'
renderLabel='readonly'
placeholder="placeholder"
/>
<NumberInput
renderLabel='with error message'
placeholder="placeholder"
messages={[{ text: 'This is an error.', type: 'error' }]}
Copy link
Collaborator

@adamlobler adamlobler Nov 29, 2025

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.

Copy link
Collaborator Author

@matyasf matyasf Dec 1, 2025

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='with success message'
placeholder="placeholder"
messages={[{ text: 'Great success!', type: 'success' }]}
/>
<NumberInput
renderLabel='large size (default is "medium")'
placeholder="placeholder"
size='large'
/>
</Flex>
```

### Guidelines
Expand Down
31 changes: 17 additions & 14 deletions packages/ui-number-input/src/NumberInput/__tests__/NumberInput.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ import '@testing-library/jest-dom'

import { NumberInput } from '../index'

import { IconZoomInLine, IconZoomOutLine } from '@instructure/ui-icons'
import {
ChevronUpInstUIIcon,
ChevronDownInstUIIcon
} from '@instructure/ui-icons-lucide'

describe('<NumberInput />', () => {
let consoleWarningMock: ReturnType<typeof vi.spyOn>
Expand Down Expand Up @@ -139,7 +142,7 @@ describe('<NumberInput />', () => {
it('shows arrow spinbuttons by default', async () => {
const { container } = render(<NumberInput renderLabel="Label" />)
const buttons = container.querySelectorAll(
'button[class$="-numberInput_arrow'
'button[class$="-numberInput_arrow"]'
)

expect(buttons).toHaveLength(2)
Expand All @@ -150,7 +153,7 @@ describe('<NumberInput />', () => {
<NumberInput renderLabel="Label" showArrows={false} />
)
const buttons = container.querySelectorAll(
'button[class$="-numberInput_arrow'
'button[class$="-numberInput_arrow"]'
)

expect(buttons).toHaveLength(0)
Expand All @@ -162,7 +165,7 @@ describe('<NumberInput />', () => {
<NumberInput renderLabel="Label" onIncrement={onIncrement} />
)
const buttons = container.querySelectorAll(
'button[class$="-numberInput_arrow'
'button[class$="-numberInput_arrow"]'
)

userEvent.click(buttons[0])
Expand All @@ -182,7 +185,7 @@ describe('<NumberInput />', () => {
/>
)
const buttons = container.querySelectorAll(
'button[class$="-numberInput_arrow'
'button[class$="-numberInput_arrow"]'
)

userEvent.click(buttons[0])
Expand All @@ -198,7 +201,7 @@ describe('<NumberInput />', () => {
<NumberInput renderLabel="Label" readOnly onIncrement={onIncrement} />
)
const buttons = container.querySelectorAll(
'button[class$="-numberInput_arrow'
'button[class$="-numberInput_arrow"]'
)

userEvent.click(buttons[0])
Expand All @@ -215,7 +218,7 @@ describe('<NumberInput />', () => {
)

const buttons = container.querySelectorAll(
'button[class$="-numberInput_arrow'
'button[class$="-numberInput_arrow"]'
)

userEvent.click(buttons[1])
Expand All @@ -235,7 +238,7 @@ describe('<NumberInput />', () => {
/>
)
const buttons = container.querySelectorAll(
'button[class$="-numberInput_arrow'
'button[class$="-numberInput_arrow"]'
)

userEvent.click(buttons[1])
Expand All @@ -251,7 +254,7 @@ describe('<NumberInput />', () => {
<NumberInput renderLabel="Label" readOnly onDecrement={onDecrement} />
)
const buttons = container.querySelectorAll(
'button[class$="-numberInput_arrow'
'button[class$="-numberInput_arrow"]'
)

userEvent.click(buttons[1])
Expand All @@ -277,19 +280,19 @@ describe('<NumberInput />', () => {
onIncrement={onIncrement}
onDecrement={onDecrement}
renderIcons={{
increase: <IconZoomInLine />,
decrease: <IconZoomOutLine />
increase: <ChevronUpInstUIIcon />,
decrease: <ChevronDownInstUIIcon />
}}
/>
)

const zoomInIcon = container.querySelector('svg[name="IconZoomIn"]')
const zoomOutIcon = container.querySelector('svg[name="IconZoomOut"]')
const zoomInIcon = container.querySelector('svg[name="ChevronUp"]')
const zoomOutIcon = container.querySelector('svg[name="ChevronDown"]')
expect(zoomInIcon).toBeInTheDocument()
expect(zoomOutIcon).toBeInTheDocument()

const buttons = container.querySelectorAll(
'button[class$="-numberInput_arrow'
'button[class$="-numberInput_arrow"]'
)

userEvent.click(buttons[0])
Expand Down
Loading