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

accesibility issue #2704

Closed

Conversation

CrisGuzmanS
Copy link
Contributor

References

Add references/links to any related issues or PRs. These may include:

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:

  • aria-haspopu and aria-expanded added in expandable-navbar-section.component.html
  • programmaticly

Include guidance for how to test or review your PR.

  1. go to https://demo.ktheia.com/
  2. put the mouse over "All of DSpace"
  3. Open de console, you should see how the aria-expanded change from false to true when mouse is over

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!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes ESLint validation using yarn lint
  • My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR fixes an issue ticket, I've linked them together.

Copy link
Member

@tdonohue tdonohue left a 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">
Copy link
Member

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);
Copy link
Member

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');
Copy link
Member

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.

@tdonohue tdonohue added bug accessibility 1 APPROVAL pull request only requires a single approval to merge medium priority port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release labels Dec 8, 2023
@davide-negretti
Copy link
Contributor

Hi @CrisGuzmanS @tdonohue
I just noticed this PR. I have just opened the PR #2676 where I refactored the whole header and navbar, and our PRs are going to conflict. However I have also handled these accessibility issues in my PR.

@tdonohue
Copy link
Member

@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.

@tdonohue tdonohue closed this Dec 12, 2023
@alexandrevryghem alexandrevryghem removed the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge accessibility bug medium priority
Projects
None yet
4 participants