Skip to content
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

Make value and defaultValue valid in number and slider fields #5543

Merged
merged 6 commits into from Dec 14, 2023

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Dec 7, 2023

Closes #4978
Closes #5556

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@rspbot
Copy link

rspbot commented Dec 7, 2023

@rspbot
Copy link

rspbot commented Dec 12, 2023

Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a story that connects a NumberField and Slider with the Slider bound to a min/max range and the NumberField allows any number so we can give it values outside the range? RAC Slider docs have an example that does this so we are probably fine.

${'defaultValue maxValue step'} | ${{defaultValue: 20, maxValue: 10, step: 3}} | ${'9'}
${'value minValue'} | ${{value: 20, minValue: 50}} | ${'50'}
${'value maxValue'} | ${{value: 20, maxValue: 10}} | ${'10'}
${'value minValue step'} | ${{value: 20, minValue: 50, step: 3}} | ${'50'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've forgotten why this doesn't clamp to 51. Is it that the step interval start at zero or min?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

steps start at the minValue

@snowystinger
Copy link
Member Author

Should we add a story that connects a NumberField and Slider with the Slider bound to a min/max range and the NumberField allows any number so we can give it values outside the range? RAC Slider docs have an example that does this so we are probably fine.

If RAC has it, we're fine, also we have tests for this now

Comment on lines +97 to +103
if (!isNaN(value)) {
if (!isNaN(step)) {
value = snapValueToStep(value, minValue, maxValue, step);
} else {
value = clamp(value, minValue, maxValue);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is a departure from native behavior where the initial value provided to the input is respected even if it doesn't match the step but I saw in the spec where it recommends this new behavior so I'm fine with this, feels better anyways

@rspbot
Copy link

rspbot commented Dec 14, 2023

@rspbot
Copy link

rspbot commented Dec 14, 2023

@rspbot
Copy link

rspbot commented Dec 14, 2023

@rspbot
Copy link

rspbot commented Dec 14, 2023

@rspbot
Copy link

rspbot commented Dec 14, 2023

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@snowystinger snowystinger merged commit 9df43e6 into main Dec 14, 2023
26 checks passed
@snowystinger snowystinger deleted the clamp-initial-values-of-number-and-slider branch December 14, 2023 21:39
@ktabors ktabors mentioned this pull request Dec 21, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants