-
Notifications
You must be signed in to change notification settings - Fork 296
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
feat(input-stepper): add aria-valuemin, aria-valuemax and an option t… #2423
Conversation
…o set aria-valuetext
🦋 Changeset detectedLatest commit: 0f9cff9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
min="1" | ||
max="10" | ||
name="value" | ||
@model-value-changed="${ev => onModelValueChanged(ev)}" |
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.
@model-value-changed="${ev => onModelValueChanged(ev)}" | |
@model-value-changed="${onModelValueChanged}" |
@@ -66,6 +71,8 @@ export class LionInputStepper extends LocalizeMixin(LionInput) { | |||
this.formatter = formatNumber; | |||
this.min = Infinity; | |||
this.max = Infinity; | |||
/** @type {string | undefined} */ |
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.
Maybe some description, so that this will be picked up in our api tables?
https://developer.mozilla.org/en-US/docs/Web/API/Element/ariaValueText
|
||
```js preview-story | ||
export const valueText = () => { | ||
const values = [ |
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.
Since an input step will always have a set of discrete steps and labels (value-texts) belonging to it, maybe a better api would be an index based array corresponding with the amount of steps
Otherwise you would have to do this always (there's no use case for providing just one valueText
this.setAttribute('aria-valuenow', `${displayValue}`); | ||
if (!this.valueText) { | ||
// VoiceOver announces percentages once the valuemin or valuemax are used. | ||
// This can be fixed by setting valuetext to the same value as valuenow |
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.
Do we have a source explaining this behaviour + workaround?
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.
When used with progressbar and scrollbar, assistive technologies announce the value to users as a percent. When aria-valuemin and aria-valuemax are both defined, the percent value is calculated as a position on the range. Otherwise, the actual value is announced as a percent.
When the value to be announced, either the actual value or the value as a percent, may not be clear to users, the aria-valuetext attribute should be used to provide a user-friendly representation of the value.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-valuenow
At least for VoiceOver the same goes up for spinbutton.
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. Maybe add the link in the test as well?
Co-authored-by: Oleksii Kadurin <[email protected]>
Co-authored-by: Oleksii Kadurin <[email protected]>
@@ -49,6 +49,36 @@ Use `min` and `max` attribute to specify a range. | |||
></lion-input-stepper> | |||
``` | |||
|
|||
### Value text | |||
|
|||
Use the `.valueText` property to override the value with a text. |
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.
Nice, can we rename it to .valueTextMapping
?
…o set aria-valuetext
What I did