[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
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)