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

feat(typeahead): improved accessibility #582 #5547

Merged

Conversation

samshay
Copy link
Contributor

@samshay samshay commented Nov 14, 2019

should close #582

PR Checklist

Before creating new PR, please take a look at checklist below to make sure that you've done everything that needs to be done before we can merge it.

  • read and followed the CONTRIBUTING.md guide.
  • built and tested the changes locally.
  • added/updated tests ( no test for typeahead)
  • added/updated API documentation.
  • added/updated demos ( not necessary )

To add accessibility to typeahead, i copy the same modification of ng-bootstrap :
ng-bootstrap/ng-bootstrap@e1fa7a4#diff-2b05df9eab924379f039993fae7b433e

I test this with NVDA and it can voice correctly now.

Reference issue : #582

@codecov
Copy link

codecov bot commented Jan 10, 2020

Codecov Report

Merging #5547 into development will increase coverage by 0.05%.
The diff coverage is 73.91%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5547      +/-   ##
===============================================
+ Coverage        73.58%   73.64%   +0.05%     
===============================================
  Files              323      323              
  Lines            11647    11664      +17     
  Branches          2481     2481              
===============================================
+ Hits              8571     8590      +19     
+ Misses            1968     1966       -2     
  Partials          1108     1108
Impacted Files Coverage Δ
src/typeahead/typeahead.directive.ts 80.43% <60%> (+0.5%) ⬆️
src/typeahead/typeahead-container.component.ts 76.11% <84.61%> (+1.4%) ⬆️
src/datepicker/bs-datepicker.component.ts 69.84% <0%> (+1.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b405057...826f1aa. Read the comment docs.

@samshay
Copy link
Contributor Author

samshay commented Jan 10, 2020

What's the status of merging pull requests to this repo? I see there are more waiting.

@daniloff200 daniloff200 requested a review from valorkin January 11, 2020 11:08
@valorkin valorkin changed the title Fix typeahead accessibility #582 feat(typeahead): improved accessibility #582 Jan 14, 2020
valorkin
valorkin previously approved these changes Jan 14, 2020
Copy link
Member

@valorkin valorkin left a comment

Choose a reason for hiding this comment

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

looks solid, thanks a lot for this great contribution!

daniloff200
daniloff200 previously approved these changes Jan 14, 2020
@daniloff200 daniloff200 removed their assignment Jan 15, 2020
@samshay samshay requested a review from daniloff200 January 23, 2020 08:04
@samshay
Copy link
Contributor Author

samshay commented Feb 27, 2020

Ok, thanks, it's possible to do this with dropdowns, collapse because the code is not into the component, and the aria attribute can be add in client html code.
But for typeahead, the code (html) is inside the component, how add aria attribute in the code generated by the component (or directive ), look my pull request ?

In my advise, it's not possible ?
But I may be wrong, in any case I don't know how to do it ...
@dmitry-zhemchugov ???

IraErshova
IraErshova previously approved these changes Mar 6, 2020
@dmitry-zhemchugov
Copy link
Contributor

Tested. Ready to megre

@daniloff200 daniloff200 merged commit 00b39c4 into valor-software:development Mar 12, 2020
@daniloff200
Copy link
Contributor

Finally, we did it!
Woohoo! 😄 🍷 🍾

@samshay
Copy link
Contributor Author

samshay commented Mar 12, 2020

Thanks to @dmitry-zhemchugov @IraErshova @daniloff200 and @valorkin

@samshay samshay deleted the fix-typeahead-accessibility branch March 18, 2020 11:07
@dharitdesai
Copy link

@dmitry-zhemchugov @IraErshova @daniloff200 @valorkin @samshay
there is an issue in here,

'[attr.aria-autocomplete]': 'list'

that list is not defined.
if you try to build your project by using this using AoT compiler, it throws error in build like this
ERROR: src/location/location.html(16,9): : Directive TypeaheadDirective, Property 'list' does not exist on type 'TypeaheadDirective'.
I can throw a PR if you guys want to

dharitdesai pushed a commit to dharitdesai/ngx-bootstrap that referenced this pull request Mar 24, 2020
adding variable `list` which was added in host Object as `[attr.aria-autocomplete]: 'list'` but was not defined.
Modules/Libraries which uses AoT compilation for their build gets fatal error due to this.

this will fix let AoT builds to work properly which was introduced in MR valor-software#5547
daniloff200 pushed a commit to dharitdesai/ngx-bootstrap that referenced this pull request Mar 26, 2020
adding variable `list` which was added in host Object as `[attr.aria-autocomplete]: 'list'` but was not defined.
Modules/Libraries which uses AoT compilation for their build gets fatal error due to this.

this will fix let AoT builds to work properly which was introduced in MR valor-software#5547
daniloff200 pushed a commit that referenced this pull request Mar 27, 2020
adding variable `list` which was added in host Object as `[attr.aria-autocomplete]: 'list'` but was not defined.
Modules/Libraries which uses AoT compilation for their build gets fatal error due to this.

this will fix let AoT builds to work properly which was introduced in MR #5547
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(typeahead): add accessibility attributes
6 participants