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

A11y fixed issues #276

Merged
merged 13 commits into from
Dec 28, 2023
Merged

A11y fixed issues #276

merged 13 commits into from
Dec 28, 2023

Conversation

joselrio
Copy link
Contributor

@joselrio joselrio commented Aug 28, 2023

This PR will add several improvements for accessibility, such:

  • Fixed the disabled behaviour since when a keyboard was in use to navigate though the screen it was possible to open a disabled dropdown;
  • Improved the options structure in order to proper reflect the correct information about each one by changing the way focus was being managed;
  • Disabled tabindex=0 at all options and set it only to the one that is focused;
  • Removed live-region, once managing/update focus it will not be needed;
  • aria-label was added to all the options;
  • Disabled search input focus when there is no options to show;
  • Added the ability to clear all the selected options when user uses the backspace or delete key;

@abenhamdine
Copy link
Contributor

abenhamdine commented Aug 28, 2023

Amazing ! 🎉
Did you run the tests ?
Because it looks like there's no CI in this project which runs tests on PR (I wish I can add a workflow in a future PR if I find some time)

FI, on my windows machine, I only succedeed in running the tests by changing

- "test": "docsify serve docs -p 3001 & cypress run"
+ "test": "docsify serve docs -p 3001 | cypress run"

Without this change, the docsify never returns and the tests are not launched

@gnbm gnbm requested a review from sa-si-dev August 28, 2023 12:56
@gnbm
Copy link
Collaborator

gnbm commented Aug 28, 2023

@sa-si-dev I already took a look at the PR but I think I will need your validation here since I'm afraid of missing some tests, mostly on changes done in focusOption

@gnbm
Copy link
Collaborator

gnbm commented Aug 28, 2023

Amazing ! 🎉 Did you run the tests ? Because it looks like there's no CI in this project which runs tests on PR (I wish I can add a workflow in a future PR if I find some time)

FI, on my windows machine, I only succedeed in running the tests by changing

- "test": "docsify serve docs -p 3001 & cypress run"
+ "test": "docsify serve docs -p 3001 | cypress run"

Without this change, the docsify never returns and the tests are not launched

@abenhamdine I also got that issue in the past and also, while running npm run validate I get some issues I'm not able to fix atm.
image

@joselrio
Copy link
Contributor Author

Amazing ! 🎉 Did you run the tests ? Because it looks like there's no CI in this project which runs tests on PR (I wish I can add a workflow in a future PR if I find some time)

FI, on my windows machine, I only succedeed in running the tests by changing

- "test": "docsify serve docs -p 3001 & cypress run"
+ "test": "docsify serve docs -p 3001 | cypress run"

Without this change, the docsify never returns and the tests are not launched

Hi abenhamdine.

There is 1 test failing but I do not have too much context about Cypress...
Possible this test must be rewritten.

Cheers,
JR

@abenhamdine
Copy link
Contributor

Amazing ! 🎉 Did you run the tests ? Because it looks like there's no CI in this project which runs tests on PR (I wish I can add a workflow in a future PR if I find some time)
FI, on my windows machine, I only succedeed in running the tests by changing

- "test": "docsify serve docs -p 3001 & cypress run"
+ "test": "docsify serve docs -p 3001 | cypress run"

Without this change, the docsify never returns and the tests are not launched

@abenhamdine I also got that issue in the past and also, while running npm run validate I get some issues I'm not able to fix atm. image

Yes looks like every contributor runs into these issue, doesn't look serious.

@gnbm
Copy link
Collaborator

gnbm commented Aug 28, 2023

Amazing ! 🎉 Did you run the tests ? Because it looks like there's no CI in this project which runs tests on PR (I wish I can add a workflow in a future PR if I find some time)
FI, on my windows machine, I only succedeed in running the tests by changing

- "test": "docsify serve docs -p 3001 & cypress run"
+ "test": "docsify serve docs -p 3001 | cypress run"

Without this change, the docsify never returns and the tests are not launched

@abenhamdine I also got that issue in the past and also, while running npm run validate I get some issues I'm not able to fix atm. image

Yes looks like every contributor runs into these issue, doesn't look serious.

Basically, the solution is the same. Maybe these commands were made for someone using a Mac and are not compatible with Windows. Not sure, but I'm able to run all commands now. And in this particular case, I'm not getting any failed test

@abenhamdine
Copy link
Contributor

I ran the tests on your PR on local and indeed they all pass 👏

image

@abenhamdine
Copy link
Contributor

@abenhamdine I also got that issue in the past and also, while running npm run validate I get some issues I'm not able to fix atm.
Basically, the solution is the same. Maybe these commands were made for someone using a Mac and are not compatible with Windows. Not sure, but I'm able to run all commands now. And in this particular case, I'm not getting any failed test

hmm I don't think it's the same issue.
LF lint issue is due to eslint rule not appropriate for windows files
Perhaps if you use pipe operators you don't see this message anymore but it's probably due to the scss lint (the last task in the command line) hiding the output

Copy link
Owner

@sa-si-dev sa-si-dev left a comment

Choose a reason for hiding this comment

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

Thank you all for the contribution. I will review the code and update.

@gnbm
Copy link
Collaborator

gnbm commented Oct 8, 2023

Thank you all for the contribution. I will review the code and update.

Hey @sa-si-dev
Any ETA for a new release with these improvements reviewed?

@gnbm gnbm requested a review from sa-si-dev October 25, 2023 19:31
@gnbm
Copy link
Collaborator

gnbm commented Nov 27, 2023

@sa-si-dev Any updated about the review of this PR?

Copy link
Collaborator

@gnbm gnbm left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants