[NumberField] Fix disabled state on increment/decrement buttons #1462
Conversation
mj12albert
added
component: number field
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
mj12albert
requested review from
atomiks,
colmtuite and
michaldudak
as code owners
| @@ -315,4 +316,22 @@ describe('<NumberField.Decrement />', () => { | |||
|
|
|||
| expect(input).to.have.value('-2'); | |||
| }); | |||
|
|
|||
| it('prop: disabled', async () => { | |||
There was a problem hiding this comment.
nit: the it function should describe the correct behavior. We usually place "prop: X" on the describe blocks.
| @@ -452,7 +452,8 @@ export function useNumberFieldRoot( | |||
| }, | |||
| onPointerDown(event) { | |||
| const isMainButton = !event.button || event.button === 0; | |||
| const isDisabled = disabled || (isIncrement ? isMax : isMin); | |||
| const isDisabled = | |||
| (externalProps.disabled ?? false) || disabled || (isIncrement ? isMax : isMin); | |||
There was a problem hiding this comment.
I'm not entirely comfortable with this pattern. Not because it produces wrong results, but because we generally never rely on anything from externalProps. Is there a good reason for keeping all the buttons' handlers in the root hook instead of moving them closer to components?
There was a problem hiding this comment.
Is there a good reason for keeping all the buttons' handlers in the root hook instead of moving them closer to components?
Yeah I agree, I thought reaching into externalProps was starting to get weird 😅
Probably this was the earliest rewritten component and later we moved to have them closer to components, do you think its ok to move this to its own hook now @atomiks ?
There was a problem hiding this comment.
Yeah this was written before we moved hooks/logic into their owning components - it's okay to change it to follow the latest patterns
mj12albert
force-pushed
the
fix/disabled-numberfield-buttons
branch
6 times, most recently
from
d168d31 to
0d23753
Compare
|
I've extracted the hooks/prop getters from the root hook except for the scrub related ones to not conflict with #1485 |
github-actions
bot
added
the
PR: out-of-date
mj12albert
force-pushed
the
fix/disabled-numberfield-buttons
branch
from
0d23753 to
bc73bb5
Compare
github-actions
bot
added
PR: out-of-date
mj12albert
force-pushed
the
fix/disabled-numberfield-buttons
branch
from
9b1b572 to
9d56a4e
Compare
github-actions
bot
added
PR: out-of-date
mj12albert
force-pushed
the
fix/disabled-numberfield-buttons
branch
from
9d56a4e to
bc3f6b9
Compare
github-actions
bot
removed
the
PR: out-of-date
| const { getInputProps } = useNumberFieldInput({ | ||
| allowInputSyncRef, | ||
| autoFocus, | ||
| disabled, | ||
| formatOptionsRef, | ||
| getAllowedNonNumericKeys, | ||
| getStepAmount, | ||
| id, | ||
| incrementValue, | ||
| inputMode, | ||
| inputValue, | ||
| invalid, | ||
| max, | ||
| mergedRef, | ||
| min, | ||
| name, | ||
| readOnly, | ||
| required, | ||
| setValue, | ||
| valueRef, | ||
| setInputValue, | ||
| locale, | ||
| inputRef: forwardedRef, | ||
| }); |
There was a problem hiding this comment.
I think we can just keep the logic inside the components, as this inflates the bundle size by passing all the params (same with the other components)
There was a problem hiding this comment.
Makes sense about the buttons as they previously were shared in the root - I think it's ok then
Fixes #1443
Before: https://codesandbox.io/p/sandbox/cocky-kepler-y5j9tz
After: https://codesandbox.io/p/devbox/billowing-cookies-gnsfn5
The issue was that pointer event handlers didn't consider the disabled prop on the buttons, only other disabled states (e.g. root)
Also extracted hooks/prop getters from the root hook to individual hooks.