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

tkakar/cat-1029-fix-links-for-accessibility #3620

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

tkakar
Copy link
Collaborator

@tkakar tkakar commented Nov 25, 2024

Summary

This PR addresses the accessibility issue of Ensuring links having discernible text by adding aria-labels to any links used in the code base, particularly, the tiles and stacked barchart components.

Design Documentation/Original Tickets

CAT-1029

Testing

Manually tested with Axe DevTools

Screenshots/Video

Screenshot 2024-11-25 at 10 53 00 AM

Include screenshots or video demonstrating any significant visual or behavioral changes.

Checklist

  • Code follows the project's coding standards
    • Lint checks pass locally
    • New CHANGELOG-your-feature-name-here.md is present in the root directory, describing the change(s) in full sentences.
  • Unit tests covering the new feature have been added
  • All existing tests pass
  • Any relevant documentation in JIRA/Confluence has been updated to reflect the new feature
  • Any new functionalities have appropriate analytics functionalities added

Additional Notes

Please specify any additional information or context relevant to this PR.

return <a href={href}>{tile}</a>;
return (
<a href={href} aria-label={href}>
{tile}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if we want to parse the hrefs in this case by extracting keywords from them. But it would need some kind of mapping, easy for organs, difficult for datasets page with long uuids that are not meaningful

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto here - I think constructing a descriptive text would be more straightforward if we were to define that logic from the parent element that uses these tiles.

}

const decodedHref = JSON.parse(LZString.decompressFromEncodedURIComponent(encodedURI)) as searchObj;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took a first pass on this, this extracts the filter (assaytype, etc) and organ, we can also include the count (shown on hover) but not sure if that's important as hover does provide that info?
Also, would be happy to move it to some utils file, or adding more meaningful content to the aria-label

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we could pass the aria-label text generation logic to the stacked bar chart, then directly pass the label as a prop here? That seems simpler than reverse engineering the aria label from the href, and will work for non-search page links as well.

@tkakar tkakar changed the title Linting and type fixing tkakar/cat-1029-fix-links-for-accessibility Nov 25, 2024
Copy link
Collaborator

@NickAkhmetov NickAkhmetov left a comment

Choose a reason for hiding this comment

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

Thanks for pulling in the lang="en" fixes!

These are some great iterations over the initial version, but I think there are still more improvements which could be made here - let me know if I can clarify any of my feedback! 😄

context/app/static/js/shared-styles/charts/hooks.ts Outdated Show resolved Hide resolved
@@ -97,6 +97,7 @@ function Tile({ hit }: { hit: SearchHit<Partial<Entity>> }) {
uuid={hit?._source?.uuid}
id={hit?._source?.hubmap_id}
entityData={hit?._source}
ariaLabelText={`Tile representing ${hit?._source?.entity_type} ${hit?._source?.uuid}`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the HBM ID would be preferable to the UUID here since the screenreader would have to read off all 32 characters of the UUID.

We may also want to be more descriptive depending on the result type - e.g. right now this will say Tile representing Dataset blahblahblah or Tile representing Sample blahblahblah, but in the future we could provide e.g. the assay type and source organ for datasets, sample category and source organ for samples, and descriptive info for donors.

@@ -92,6 +92,7 @@ function ProvEntityColumnContent({
uuid={item.uuid}
id={item.hubmap_id}
entity_type={item.entity_type}
ariaLabelText={`Tile representing ${item.entity_type} ${item.uuid}`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if a useEntityTileAriaLabel hook could help? That would allow us to implement more complex logic depending on the content type (dataset/sample/donor).

Similar concern as below with using UUID vs. HBM ID in aria labels.

Copy link
Collaborator

@NickAkhmetov NickAkhmetov left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! Just a few minor finishing touches:

const organ = String(d?.bar?.data?.organ);
const assay = d.key;
const count = d.key ? d?.bar?.data[d.key] : null;
return `${count} ${organ} datasets with assay type ${assay}.`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should account for null's and undefined's string representations in template strings being "null"/"undefined" e.g.:

const test = `${null} ${"Stomach"} datasets with assay type ${"scRNA-seq"}` 
console.log(test)
> "null Stomach datasets with assay type scRNA-seq"
const test2 = `${undefined} ${"Stomach"} datasets with assay type ${"scRNA-seq"}` 
console.log(test2)
> "undefined Stomach datasets with assay type scRNA-seq"

Maybe this could work?

// We can use optional chaining with indexed objects like this:
const count = d?.bar?.data?.[d.key];
if (count) {
  return `${count} ${organ} datasets with assay type ${assay}.`;
}
return `${organ} datasets with assay type ${assay}.`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants