search

codeforpdx / PASS

PASS project - with HMIS module integration
MIT License
30 stars 26 forks source link

Improve accessible name for images on home page and prop names for HomeSection #458

Closed milofultz closed 11 months ago

milofultz commented 11 months ago

This PR:

Improves accessible name for images on home page.

1. Adds context for all logo images 2. Removes unnecessary aria-labels 3. Mark decorative (non-informative images) as such using an empty alt attribute

The files this PR effects:

Components

src/components/Footer/RenderCompanyInfoSection.jsx
src/components/NavBar/NavbarDesktop.jsx
src/components/NavBar/NavbarLoggedOut.jsx
src/components/NavBar/NavbarMobile.jsx
src/pages/Home.jsx

Tests

test/components/NavBar/NavbarDesktop.test.jsx
test/components/NavBar/NavbarLoggedOut.test.jsx
test/components/NavBar/NavbarMobile.test.jsx
test/pages/__snapshots__/Home.test.jsx.snap

Screenshots (if applicable):

Should be no difference visually.

Additional Context (optional):

Following guidance around decorative/informative images from W3. Verified using this extension and DevTools.

xscottxbrownx commented 11 months ago

@leekahung I believe this is ready for you to approve now - so merging is possible.

milofultz commented 11 months ago

Yeah, I'm mostly fine with this. Although I've notice the image alts for HomeSection are all empty now compared to before. Won't it be necessary for accessibility?

https://accessibility.psu.edu/images/imageshtml/

I went with the info about decorative/informative images from W3 (see example 4 in decorative as that's what I thought it fell under). It was a judgment call that I decided they were decorative since I'm not sure they contribute to the content. Happy to hear otherwise, it's a tough call.

leekahung commented 11 months ago

I went with the info about decorative/informative images from W3 (see example 4 in decorative as that's what I thought it fell under). It was a judgment call that I decided they were decorative since I'm not sure they contribute to the content. Happy to hear otherwise, it's a tough call.

Ah, I see. Well, considering that the section title follows immediately after the images, I think it should be fine even if the image themselves break. Alright, I'll approve this.

leekahung commented 11 months ago

Hey @milofultz. Was planning to merge this branch in after resolving a merge conflict for one of the test files.

Unfortunately, the resolution I've attempted to make seemed to have failed the test. I think you'll be able to fix it from your end (sorry for the inconvinence). If the test gets fixed, let me know, I'll have this merged into Development.

Thanks!

milofultz commented 11 months ago

@leekahung Should be good to go now 👍