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

Added code and test to clear selected field using backspace #52

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Suvra19
Copy link

@Suvra19 Suvra19 commented Dec 2, 2018

Adding functionality to clear selected field using keyboard to support accessibility standards.

"Some people cannot use a mouse, including many older users with limited fine motor control. An accessible website does not rely on the mouse; it makes all functionality available from a keyboard."
source: https://www.w3.org/standards/webdesign/accessibility

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 81.45% when pulling d8f2157 on mediasuitenz:add-keyboard-support-to-clear-selected-option into e26f490 on selvagsz:master.

@coveralls
Copy link

coveralls commented Dec 2, 2018

Coverage Status

Coverage increased (+0.3%) to 81.681% when pulling 27bbe91 on mediasuitenz:add-keyboard-support-to-clear-selected-option into e26f490 on selvagsz:master.

@selvagsz
Copy link
Owner

selvagsz commented Dec 4, 2018

Is this a normal behavior in selects ? Can you cite some examples that have this behavior ?

Also, the disabled should be respected. Meaning, when the powerselect is disabled, pressing backspace should not clear it

@Suvra19
Copy link
Author

Suvra19 commented Dec 4, 2018

Yes. Actually it's a common practice to have valid keyboard alternatives to meet accessibility standards. For our case, just clearing the selected field using keyboard was missing. Example: https://jedwatson.github.io/react-select/

And yes, disabled check is a good catch. Thanks.

@Suvra19
Copy link
Author

Suvra19 commented Dec 4, 2018

@selvagsz I have added the check and also a test case, just in case.

@Suvra19
Copy link
Author

Suvra19 commented Dec 5, 2018

@selvagsz sorry to bother you again, did you have a chance to look at this PR ? I have a dependency on this little functionality. Thanks!

@selvagsz
Copy link
Owner

selvagsz commented Dec 5, 2018

We have showClear to show the close icon on the rhs of the powerselect. Setting it to false will allow to you change the value but you can't clear them.

But with backspace, this gets bypassed as well

I'm still unsure whether this behavior should respect showClear option or should behind a clearOnBackspace flag

Though I hate to increase the api surface.

@selvagsz
Copy link
Owner

selvagsz commented Dec 5, 2018

However, you can achieve the same functionality by composing the component & handling the keyDown event.

I'd prefer to wait for more feedbacks on this feature

@Suvra19
Copy link
Author

Suvra19 commented Dec 11, 2018

However, you can achieve the same functionality by composing the component & handling the keyDown event.

I'd prefer to wait for more feedbacks on this feature

No worries. Thanks for letting me know.

On another note. The TetherComponent changes are merged, so when can I expect the next release ?

@Suvra19
Copy link
Author

Suvra19 commented Dec 11, 2018

We have showClear to show the close icon on the rhs of the powerselect. Setting it to false will allow to you change the value but you can't clear them.

But with backspace, this gets bypassed as well

I'm still unsure whether this behavior should respect showClear option or should behind a clearOnBackspace flag

Though I hate to increase the api surface.

So, the way I see it, backspace is just an alternate way of clearing the selected field. So any scenario where you should not be able to clear the selected field, the backspace should honour that. I am making the changes right now to not bypass the showClear flag. Since you know the api best, let me know if you see any problem with that.

Also, I don't believe there is a need to use another flag for this specifically as that will be confusing for the user.

@selvagsz
Copy link
Owner

@Suvra1990 published 1.0.0-beta.15 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants