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

aria-selected added to Item Edit "Status" Tab. Fixed ID: 470076 #2063

Merged
merged 2 commits into from
Feb 7, 2023

Conversation

CrisGuzmanS
Copy link
Contributor

@CrisGuzmanS CrisGuzmanS commented Jan 30, 2023

Hi @tdonohue ✋, I am submitting this pull request as a solution to activity ID 470076 in issue 1190. Regarding "Selected state of the element is missing or incorrect."

References

Description

in each of the tabs of the page that opens when editing an item, an aria-selected has been added, if the tab is active, it will have the value of "true", if not, it will have the value of "false".

Instructions for Reviewers

List of changes in this PR:

  • [attr.aria-selected]="page.page === currentPage" added in edit-item-page.component.html

To test my PR:

  1. Login as adminsitrator
  2. Edit any item
  3. check the value of aria-selected of each li tag in the displayed tab.
    image

Checklist

  • 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 includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
  • If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • If my PR fixes an issue ticket, I've linked them together.

@tdonohue tdonohue added bug 1 APPROVAL pull request only requires a single approval to merge labels Feb 1, 2023
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 again! This looks good & it works. However, I realized my recommendations were missing the Aria "role" attributes that I believe these tabs still require. If you could add them in this PR as well, that'd be appreciated. See my inline comment below.

(If you don't have time to make these small changes, let me know. We can always move this PR forward as-is and see if someone else can add the "role" attributes)

@@ -4,7 +4,7 @@
<h2 class="border-bottom">{{'item.edit.head' | translate}}</h2>
<div class="pt-2">
<ul class="nav nav-tabs justify-content-start">
<li *ngFor="let page of pages" class="nav-item">
<li *ngFor="let page of pages" class="nav-item" [attr.aria-selected]="page.page === currentPage">
Copy link
Member

Choose a reason for hiding this comment

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

I think this looks good! However, I just realized that the recommendation for tabs is to set the roles "tablist" and "tab". See the docs at https://www.w3.org/WAI/ARIA/apg/patterns/tabs/

So, I think we need to add two more small things here.

  • First, on the <ul> just above this we should have <ul role="tablist"..
  • Seconding, on this <li> we should add the <li role="tab"..

Once those are in place, I think this aligns better with the recommendations in the docs above.

@CrisGuzmanS
Copy link
Contributor Author

@CrisGuzmanS : Thanks again! This looks good & it works. However, I realized my recommendations were missing the Aria "role" attributes that I believe these tabs still require. If you could add them in this PR as well, that'd be appreciated. See my inline comment below.

(If you don't have time to make these small changes, let me know. We can always move this PR forward as-is and see if someone else can add the "role" attributes)

@tdonohue i can work on it. Tomorrow i will update the code

i have added roles for ul and li (tablist and tab)
@CrisGuzmanS
Copy link
Contributor Author

@tdonohue i already did the changes you recommended me. Please, let me know if everything is ok 👍

@tdonohue tdonohue self-requested a review February 2, 2023 22:36
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.

👍 Thanks @CrisGuzmanS ! Finally got back to testing this & I can verify this fixes that first issue in #1190. Will merge immediately

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
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants