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

RangeControl > Value entry field (optional): #15931

Closed
AlchemyUnited opened this issue May 31, 2019 · 9 comments · Fixed by #21695
Closed

RangeControl > Value entry field (optional): #15931

AlchemyUnited opened this issue May 31, 2019 · 9 comments · Fixed by #21695
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Developer Documentation Documentation for developers

Comments

@AlchemyUnited
Copy link

The README for RangeControl says the Value entry input is optional.

https://github.com/WordPress/gutenberg/tree/master/packages/components/src/range-control

I presume -wrongly? - that mean there's a prop for being able to not display this input. But that doesn't seem to be the case. That bit isn't wrapped in any conditional.

			{ afterIcon && <Dashicon icon={ afterIcon } /> }
			<input
				className="components-range-control__number"
				type="number"
				onChange={ onChangeValue }
				aria-label={ label }
				value={ currentInputValue }
				min={ min }
				max={ max }
				onBlur={ resetCurrentInput }
				{ ...props }
			/>
			{ allowReset && (
@swissspidy swissspidy added [Package] Components /packages/components [Type] Developer Documentation Documentation for developers labels May 31, 2019
@swissspidy
Copy link
Member

Yeah that seems incorrect.

Introduced in #12564.

@swissspidy swissspidy added the Good First Issue An issue that's suitable for someone looking to contribute for the first time label May 31, 2019
@talldan
Copy link
Contributor

talldan commented May 31, 2019

I don't think the documentation should be updated to remove this. This is part of the design guidelines, and in terms of a design, this use of the RangeControl is feasible.

It's technically not possible yet, probably because no RangeControl has ever been implemented in Gutenberg in this way.

I'll add the Needs Design Feedback to double check that this documented design guideline is correct. If it is, the right thing would be to improve the component to allow optional rendering of the input.

@talldan talldan added the Needs Design Feedback Needs general design feedback. label May 31, 2019
@swissspidy swissspidy added the Needs Accessibility Feedback Need input from accessibility label May 31, 2019
@swissspidy
Copy link
Member

I don't think a RangeControl without a number input is very accessible, adding a label for that as well.

@talldan
Copy link
Contributor

talldan commented May 31, 2019

Yep, that seems sensible. It's a native browser input with type of 'range', but I suppose that still doesn't rule out it not being accessible, plenty of native browser things are not accessible.

@AlchemyUnited
Copy link
Author

@swissspidy - Let's presume you're correct about accessibility. So then "removing" it would simply mean wrapping it an sr-only class.

fwiw, I've remixed the FontSizePicker. I'm using it to select animation iterations. That is, the "select" (i.e., the dropdown menu of buttons) is the "suggested" values. For example, 1, 2, 3, 5, 7, 9 and Infinite. When Infinite is selected, I hide the number input because you can -- / ++ it.

But if I also want to use the slider (it's optional for this block / component). There's currently no way to "turn off" the number input for the slider - unless I hid the whole slider if Infinite is selected.

I think (read: I've experimented with) there are other use cases where the range control can be used not for numbers but as an "advanced" toggle of sorts. For example, 3 "toggle" settings, not simply 2. Again, that number input - in this cases - becomes unnecessary.

The point is, as I'm often finding out, the FontSizePicker control > Range control has limited flexibility, limited to a fault.

@swissspidy
Copy link
Member

Let's presume you're correct about accessibility. So then "removing" it would simply mean wrapping it an sr-only class.

Well, accessibility is more than just adding things for screen readers 🙂

@afercia
Copy link
Contributor

afercia commented Jun 1, 2019

@swissspidy makes a good point. It's not just about screen readers. Users have their specific needs. Just to name a few: motor impairments, cognitive impairments, or simply users that don't understand how to use a range control and prefer a simple input. Also from an UI perspective, I'm not sure assuming a range control provides a better experience than a number input is correct.

Worth also noting the Range control is being discussed in #14166. No great progress there so far, but it's clear there are cases where the Range control shouldn't be used at all.

For now, the recommended usage is to always have a number field coupled with the range control. Removal of the number field shouldn't be allowed.

@talldan is it possible to enforce this via code and make it clear in the documentation?

@afercia afercia added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Accessibility Feedback Need input from accessibility labels Jun 1, 2019
@talldan
Copy link
Contributor

talldan commented Jun 4, 2019

Ok, does actually seems like the best option is to update the design docs to remove the suggestion that the number input field is optional. Hopefully a designer can also approve that decision.

@talldan is it possible to enforce this via code and make it clear in the documentation?

Hi @afercia, it does already seem to be enforced by the component, the number input could only really be hidden by css. I'm not sure if it's worth going further.

@melchoyce melchoyce removed the Needs Design Feedback Needs general design feedback. label Oct 15, 2019
@melchoyce
Copy link
Contributor

Hopefully a designer can also approve that decision.

Approved! Let's get a PR for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants