-
Notifications
You must be signed in to change notification settings - Fork 437
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
accesibility issue #2704
accesibility issue #2704
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CrisGuzmanS : Thanks for the contribution. I think there's an easier way to get the aria-expanded
attribute to work though. You shouldn't need to change its value in a more "manual" fashion in the code. See comments inline.
(mouseleave)="deactivateSection($event)"> | ||
(mouseleave)="deactivateSection($event)" | ||
aria-haspopup="menu" | ||
aria-expanded="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than hardcoding this to false
and changing the value in the component.ts
code, I think you can just use isActive
here (similar to the other attributes on this same <div>
. Something like this should work:
[attr.aria-expanded]="isActive"
I haven't tested it, but you shouldn't need to hardcode this.
@@ -39,11 +39,17 @@ export class ExpandableNavbarSectionComponent extends NavbarSectionComponent imp | |||
* @param {Event} event The user event that triggered this function | |||
*/ | |||
activateSection(event): void { | |||
|
|||
console.log(event.target); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it was for debugging & should be removed.
super.activateSection(event); | ||
targetElement.setAttribute('aria-expanded', 'true'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a bunch of failures here from yarn lint
. It looks like you have trailing spaces on several lines in this file. Those should be removed until you get yarn lint
to pass locally.
Hi @CrisGuzmanS @tdonohue |
@davide-negretti : Thanks for pointing out the conflicts. Since this PR isn't complete & yours is, I'm going to close this PR in favor of #2676 . Apologies @CrisGuzmanS , as I didn't notice the conflict earlier myself when I gave this a quick review. |
References
Add references/links to any related issues or PRs. These may include:
pr-number
(if a REST API PR is required to test this)Description
Short summary of changes (1-2 sentences).
Instructions for Reviewers
i added attributes in the html component, and the aria-expanded equals to true.
List of changes in this PR:
Include guidance for how to test or review your PR.
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
yarn lint
yarn check-circ-deps
)