[MWPW-169395] - Table aria labels missing #3875
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There was a problem hiding this comment.
Looking good for the most part, I also checked this on https://table-aria-labels--milo--adobecom.hlx.page/drafts/sarchibeque/table/add-on?martech=off, where the logic for the special labels was originally added.
I just have a couple of notes for now.
mokimo
approved these changes
Apr 2, 2025
overmyheadandbody
approved these changes
Apr 2, 2025
|
Reminder to set the |
DKos95
force-pushed
the
table-aria-labels
branch
from
d8c4800 to
76684da
Compare
robert-bogos
approved these changes
Apr 7, 2025
zagi25
approved these changes
Apr 9, 2025
NadiiaSokolova
approved these changes
Apr 9, 2025
There was a problem hiding this comment.
Verified. Ready for Stage.
Testing details https://jira.corp.adobe.com/browse/MWPW-169395
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This resolves the issue where aria-labels were missing from Table CTA's, the main problem here was that Table.js and Merch.js are being executed in parallel and in the Table.js we were earlier using innerHtml to change the Table when the user resized or filtered the the tables in mobile mode, this caused the aria-labels that were being set in Merch.js to be set on the old table which was being replaced with a new table in Table.js, also columns were being removed from the DOM in curtain situations which was an additional problem causing the removal of aria-labels.
Dev Notes
Leaving this part in
.table .hide-mobile { display: none !important; }. We would need to list at least 5 classes with high css specificity to override the display properties of some elements that need to be hidden, seems to be a safer option here and less lines.QA Notes
It would be a good idea to test all the functionalities of the table since with this PR we are no longer using innerHtml which was at the core of the tables functionalities, especially filtering on mobile and the resizing of the screen.
List of Pages where Tables are present
Resolves: MWPW-169395
Test URLs:
CC Merch Test URLs
Before: https://main--cc--adobecom.hlx.page/cc-shared/fragments/merch/products/photoshop/compare-plans/table/individual/default?martech=off
After: https://main--cc--adobecom.hlx.page/cc-shared/fragments/merch/products/photoshop/compare-plans/table/individual/default?milolibs=table-aria-labels&martech=off
Before: https://main--cc--adobecom.hlx.page/cc-shared/fragments/merch/products/photoshop/compare-plans/table/edu/default?martech=off
After: https://main--cc--adobecom.hlx.page/cc-shared/fragments/merch/products/photoshop/compare-plans/table/edu/default?milolibs=table-aria-labels&martech=off