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

Pass WAI-ARIA states into dropdown #433

Merged

Conversation

jmacqueen
Copy link
Contributor

This will pass the following WAI-ARIA relationship and widget attributes through to basic-dropdown in an attempt to complete #293

I have a thing about alphabetizing long property lists to help out when exploring or debugging, I hope that doesn't cause too much trouble. Other than alphabetizing, these are the only actual additions to the templates.

power-select attribute WAI-ARIA attribute in DOM Note
ariaDescribedBy aria-describedby
ariaInvalid aria-invalid
ariaLabel aria-label This one still needs to be added to basic-dropdown, but inclusion here ahead of time doesn't break anything
ariaLabelledBy aria-labelledby
required aria-required

I didn't include tests because I'm a bad person. And I didn't want to take the time on the off-chance these changes don't go in 😄

@@ -67,7 +67,6 @@
"ember-cli-babel": "^5.1.5",
"ember-cli-htmlbars": "^1.0.1",
"ember-basic-dropdown": "^0.9.5-beta.10",
"ember-hash-helper-polyfill": "^0.1.0",
Copy link
Owner

Choose a reason for hiding this comment

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

I agree that I not longer need this polyfill, but it concerns me that this test is not failing in 1.13 in travis.

Nothing for your to change, just a note to myself.

@cibernox
Copy link
Owner

cibernox commented Apr 5, 2016

I don't mind the alphabetically order. I started grouping properties "conceptually", but clearly there was a moment where that stopped scaling 😅 . This is better now.

This does need tests tho (and probably some work in ember-basic-dropdown itself).

@cibernox
Copy link
Owner

cibernox commented Apr 5, 2016

Tests in ember-basic-dropdown and ember-power-select overlap a bit, but I try to have tests in EPS for behaviours that are anyway tested in EBD because sometimes they depend of EPS forwarding the proper properties, so in this case the duplication has a purpose.

@jmacqueen
Copy link
Contributor Author

I've ended up duplicating in similar ways (and for similar reasons) in the widget set I'm making at work. It's helped to catch refactoring problems when I thought I was safely changing something isolated in one component...and it ends up I was wrong!

Especially considering the case of integration tests, in my opinion, it's well worth having all the relevant tests for the EPS interface in the EPS repo...even if it means tests aren't technically as DRY as they could possibly be.

@jmacqueen
Copy link
Contributor Author

Tests added. I went ahead and put in tests for aria-label that will fail until ariaLabel is added to cibernox/ember-basic-dropdown and the updated version included. But it shouldn't break anything to go ahead and merge this as-is.

@cibernox
Copy link
Owner

cibernox commented Apr 5, 2016

@jmacqueen mmm... why travis is not running for the last commit? Also, should we update ember-basic-dropdown first?

@cibernox
Copy link
Owner

cibernox commented Apr 6, 2016

Rebase over master to get the version of EPS with support for aria-label.

@jmacqueen jmacqueen force-pushed the 293_pass_aria_states_into_dropdown branch from 5bac03f to efab2d5 Compare April 6, 2016 14:12
@cibernox cibernox merged commit 108f4c0 into cibernox:master Apr 6, 2016
@cibernox
Copy link
Owner

cibernox commented Apr 6, 2016

Thanks for all the a11y work.

This starts to look complete.

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

2 participants