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

Fix for critical screen reader issues, some accessibility enhancements #249

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wilwong89
Copy link
Contributor

@wilwong89 wilwong89 commented Jan 28, 2025

Description

Updated header and navbar to streamline screenreader flow
Updated submission and enquiry intake forms to have proper section headings, proper stepheader buttons
Updated permits detailed page to read out correct statuses
Fix beginning splash page header to read as one sentence
Added alt text to proponent tables
Remove dividers from being read out by screen reader.

PADS-311

Types of changes

New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Copy link

Coverage Report (Application)

Totals Coverage
Statements: 36.78% ( 1104 / 3002 )
Methods: 26.82% ( 136 / 507 )
Lines: 48.73% ( 728 / 1494 )
Branches: 23.98% ( 240 / 1001 )

Copy link

github-actions bot commented Jan 28, 2025

Coverage Report (Frontend)

Totals Coverage
Statements: 46.03% ( 3258 / 7078 )
Methods: 35.29% ( 426 / 1207 )
Lines: 53.85% ( 1886 / 3502 )
Branches: 39.93% ( 946 / 2369 )

Copy link

github-actions bot commented Jan 28, 2025

@@ -10,6 +10,6 @@ const { title } = defineProps<{
<template>
<div class="col-span-12 mb-4 flex items-center">
<h4 class="flex-none m-0 pr-4">{{ title }}</h4>
<Divider />
<Divider aria-hidden="true" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to wrap this component into our own to ensure accessibility requirements aren't missed in the future? It appears numerous times in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make the switch. We'll still need to make sure devs remember to use the our wrapper version instead of the primevue lib.

@wilwong89 wilwong89 force-pushed the feature/screenreader-updates branch from ef66d29 to 5cf1401 Compare January 29, 2025 02:02
>
{{ slotProps.item.text }}
</div>
<div
v-if="slotProps?.item?.iconClass == 'previous'"
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if it's dumb, but can we have constants declared and check it here? for previous, empty and current?

Copy link
Member

Choose a reason for hiding this comment

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

The div with the v-if v-else-if stuff is making me wonder if the actual content values inside the div should be yielded as a computed or function instead and put in as a template string, instead of remounting the entire div object - it seems like all the other div attributes are basically identical otherwise.

Comment on lines +19 to +23
<h1 class="mb-0">
Welcome to the
<br />
Permit Connect Services
</h1>
Copy link
Member

Choose a reason for hiding this comment

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

Is this something that needs to be in i18n?

>
{{ slotProps.item.text }}
</div>
<div
v-if="slotProps?.item?.iconClass == 'previous'"
Copy link
Member

Choose a reason for hiding this comment

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

The div with the v-if v-else-if stuff is making me wonder if the actual content values inside the div should be yielded as a computed or function instead and put in as a template string, instead of remounting the entire div object - it seems like all the other div attributes are basically identical otherwise.

@naixin-zhangbc
Copy link

naixin-zhangbc commented Jan 30, 2025

Looking great! One small change I'd like to suggest:

  • On permit details page, when the tracker is designed to be skipped in screen reader because the application is abandoned/withdrawn/etc., can we also skip the header "Application progress"?
  • I have a couple questions about other parts. Will chat about it in our 1:30 meeting.

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.

5 participants