-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add format
prop to RAC Slider to support arbitrary aria-valuetext
.
#6411
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: main
Are you sure you want to change the base?
Conversation
…text` customization.
*/ | ||
export function useNumberFormatter(options: NumberFormatOptions = {}): Intl.NumberFormat { | ||
export function useNumberFormatter(format?: NumberFormatOptions): Intl.NumberFormat |
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 think this is a little too low to be doing the change. We'd end up with problems in NumberFields if we exposed this there
format?: Intl.NumberFormatOptions | ((value: number, locale: string) => string), | ||
/** | ||
* The display format of the value label. | ||
* @deprecated Use `format` 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.
Likewise, I don't think we should deprecate this since it matches NumberField and the two components are frequently used together to allow either a slider or text input method to choose a number.
I understand the conflict in that statement and what you're trying to achieve here, which is clever.
I'll see if anyone else on the team has thoughts.
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'll see if anyone else on the team has thoughts.
Any updates on this?
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.
Apologies, I let it slip. I've posted it now and will hopefully have an update soon.
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.
Yeah, I think it would be good to maintain consistency with our other components that also use formatOptions. That said, I do think it would be good to support this so not really sure how we would go about this without making changes to formatOptions.
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 your patience, I'm bringing this up again.
GET_BUILD |
8cb1a5d
to
3013156
Compare
Discussed in the team today, we have a number of opposing opinions and have not settled on the API we'd like for this yet. Thank you for your patience. Some requirements we have so far:
Some other options we talked about
Something to keep an eye out for, when making a range such as https://react-spectrum.adobe.com/react-aria/Slider.html#reusable-wrappers but with labels like this I'll be marking this as "on hold" until we can get the API resolved. |
Glad to hear it’s being discussed.
Aren’t these requirements in direct contradiction? If there needs to be only one way to set the label, and you’re committed to keeping the existing formatOptions/NumberFormat mechanism for setting the label, and you want to make it possible to set the label to values other than those that can be handled by the existing mechanism… Maybe I’m missing something, but I can’t see what design could possibly satisfy all of those contraints.
I think part of a sentence got cut off? |
Yep, this is most of the source of our conflict |
a79adcf
to
3013156
Compare
Closes #6402
The aria-valuetext attribute specifies the human-readable representation of a numeric value. RAC components that allow users to manipulate numeric values should provide a means of setting
aria-valuetext
on the appropriate underlying element.Currently, RAC sliders are implemented using one
input
element per thumb. TheSlider
component takes aformatOptions
prop; thearia-valuetext
of eachinput
is computed as(The formatted values of all the thumbs in a slider are also used to set the default content of the
SliderOutput
component; this default can be overriden by providing children to the output component, whereas thearia-valuetext
of each thumb presently can't be.)The current approach is probably flexible enough for most use cases, but falls somewhat short of the spec, which gives the following example:
Currently, there is no way to implement this in RAC.
This PR adds a
format
prop toSlider
that accepts either aNumberFormatOptions
object (the same as the currentformatOptions
prop) or a function with the signature(value: number, locale: string) => string
. Ifformat
is an object, then the behavior will be the same as ifformatOptions
were specified with that object. Ifformat
is a function, then that function will be used to set thearia-valuetext
for each input, as well as the default content ofSliderOutput
.This approach makes only a modest change to the code, and enables many use cases. A few examples:
Mapping values to strings, as in the spec's
small
,medium
, andlarge
example.Representing numerical values in ways not supported by
Intl.NumberFormat
: time intervals (e.g.2h15m
), magnifications (e.g.10x
), fractions (e.g.½
), etc.Providing multiple representations of a single value, such as a temperature represented in both degrees Celsius and Fahrenheit.
Implementing non-linear scaling. It is currently almost possible to implement a non-linear scale by wrapping the existing
Slider
component, e.g.This is a working logarithmically scaled slider in every respect except the incorrect
aria-valuetext
on the underlying input.(There is an existing issue, custom scale for useSlider #1830, addressing non-linear scaling; a design was approved two years ago, but never implemented. This PR provides an alternative solution to the problem.)
I've included a storybook entry for testing the new functionality. I'm happy to add tests and documentation updates, but I wanted to get some feedback on this PR before I committed to that work.
Edit: I noticed after creating this PR that Spectrum sliders already specify a
getValueLabel
function for customizing a slider's output; as implemented, it does not correctly setaria-valuetext
, as can be observed by turning on a screen reader and looking at the "donuts to buy" example in the documentation. If this PR is merged, then Spectrum could be modified to havegetValueLabel
correctly propagate its computed label toaria-valuetext
. I'd also be happy to change the name of the prop in this PR togetValueLabel
for consistency with Spectrum, if the maintainers prefer.✅ Pull Request Checklist:
📝 Test Instructions:
http://localhost:9003/?path=/story/react-aria-components--slider-custom-format&args=showAlternative:!true&providerSwitcher-express=false&strict=true
celsius-fahrenheit-slider.mov