Add modal glossary of payment methods #181

oyamauchi · 2024-07-22T21:59:08Z

Description

There's a lot going on here, so here's the high-level tour:

The glossary modal itself looks (I think) exactly as designed. I've
made a few edits to the copy compared to what's in the Figma; I've
put comments in the Figma to explain why.
The accordion sections use Headless UI's Disclosure component.
Most of the logic of displaying a modal dialog is hand-written,
though I added third-party packages to trap focus within it and stop
the body from scrolling while it's up.

I did try using Headless UI's Dialog component, which does all
that out of the box, but (like the rest of Headless!) it doesn't account
for shadow DOMs. It renders the dialog at the root of document,
where it does not get any of the shadow DOM's styling; there seems
to be no way to change this.
The entire incentive-type chip is now a button. (The alternative was
to make only the icon the button, which makes for an uncomfortably
small click target.) This required a bit of refactoring of the
incentive card component.

Accessibility

Keyboard navigation:
- When you activate one of the chip buttons, focus automatically
  goes to the section header of the appropriate section. (The entire
  header is an expand/collapse button, not just the circled
  chevron.)
- The modal traps focus while it's up.
- Hitting Escape while the modal is up will dismiss it.
- When the modal closes, focus returns to the chip button that
  caused it to open.
Screenreader experience: I think it's OK? I am not an expert screen
reader user by any means. It's possible a few more things could use
ARIA roles.

One judgment call I made is to aria-label the question icon in the
chips as "show glossary". When a chip is focused, the reader says
"Tax credit show glossary, button". It seemed good to have a clue as
to what will happen if you click the button?

Next steps

If this all seems suitable, I'll request translations.

Test Plan

Look at the component on all widths, in several browsers (tried
Chrome, Firefox, Safari, MobileSafari). Make sure the section
corresponding to the incentive type you clicked on is open as soon
as the modal appears, and no others are open. Click on others to
open and close. Open all the sections and make sure the scrolling is
correct.
Make sure the modal is dismissible by clicking away from the body,
clicking the X button, or hitting Escape.
Scroll-wheel with the cursor away from the modal body, and make sure
the document doesn't scroll underneath.
Navigate with keyboard: tab to the incentive-type chip and hit
Enter. Make sure focus is immediately on the right section
header. Tab and shift-tab and make sure focus is trapped within the
modal. Hit Esc, or "enter" on the X button, and make sure focus is
back on the original chip.
Do all of the above with VoiceOver on.

vercel · 2024-07-22T21:59:12Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
embed-rewiringamerica-org	✅ Ready (Inspect)	Visit Preview	Sep 9, 2024 10:37pm

matelau · 2024-07-23T18:12:51Z

src/glossary.tsx

+          />
+        </span>
+      </Disclosure.Button>
+      <Disclosure.Panel className="flex flex-col gap-4">


Re: VoiceOver - Should the focus go to the button or go to the panel with the content? I'm not the best with VoiceOver, but I have used it before and I was having some issues getting VoiceOver to read the content as it was just iterating through the buttons and not making it to the glossary content.

I think there is some controversy over setting tabindex to do this, but that is possibly one way? https://webaim.org/techniques/keyboard/tabindex

But here is the applicable WCAG standard https://www.w3.org/WAI/WCAG21/Understanding/focus-order.html

It works for me in my main browser (Brave; Chromium-based). But I wouldn't be surprised if other browsers behave differently; I'm consistently surprised at how much variety there is in how browsers build the accessibility tree.

Also, the content is just plain <p>s with text inside; I can't imagine any browser would ignore those?

veekas

voice over seemed good to me

veekas · 2024-07-25T18:29:54Z

src/glossary.tsx

+          <CircledChevron
+            w={40}
+            h={40}
+            fillClass="fill-[#ebebeb] group-data-open:fill-color-action-primary"


nit, it's slightly different, but can we use grey-200 here instead? It has a Delta E ratio (i.e. color difference) of 2, which is only barely perceptible

Sure -- I just took ebebeb from the designs; I don't know why Emiola went outside the design system colors though.

veekas · 2024-07-25T18:39:13Z

src/glossary.tsx

+          // Prevent clicks on the body from closing the modal
+          onClick={e => e.stopPropagation()}


the "body" as in the rest of the page under the modal? if so, it appears to close the modal on click.

I meant the body of the modal itself -- the white-background div. Only clicks on the blurry background should close the modal. I can clarify that.

veekas · 2024-07-25T18:55:59Z

src/state-incentive-details.tsx

+          aria-label={msg('Show glossary')}
+          w={10}
+          h={10}
+          opacity={1.0}


probably redundant, unless there's some inheritance going on that I'm missing

It's because the same QuestionIcon component is used for the tooltip icons in the form, where it has to be 50% opacity. I guess I should make it so 100% is the default, though, and the usage in the form has to specify 50% explicitly. I'll change that.

veekas · 2024-07-25T19:56:53Z

src/state-incentive-details.tsx

+                <button
+                  type="button"


should we use the Button component? either way, I don't think we need the type attribute here

There's no Button component in this codebase -- it doesn't use a library of styled components at all. There are a couple of reusable things in src/buttons.tsx, but none of those are applicable here.

Also, the default <button> type is submit, which is not what we want here. (Although this button isn't within a form, so submit and button would have the same behavior anyway, but I'm just in the habit of writing type=button.)

## Description There's a lot going on here, so here's the high-level tour: - The glossary modal itself looks (I think) exactly as designed. I've made a few edits to the copy compared to what's in the Figma; I've put comments in the Figma to explain why. - Most of the logic of displaying a modal dialog is hand-written, though I added third-party packages to trap focus within it and stop the body from scrolling while it's up. I did try using Headless UI's `Dialog` component, which does all that out of the box, but (like the rest of Headless!) it's unusable with shadow DOM. It renders the dialog at the root of `document`, where it does not get any of the shadow DOM's styling; there seems to be no way to change this. - The entire incentive-type chip is now a button. (The alternative was to make only the icon the button, which makes for an uncomfortably small click target.) This required a bit of refactoring of the incentive card component. ### Accessibility - Keyboard navigation: - When you activate one of the chip buttons, focus automatically goes to the section header of the appropriate section. (The entire header is an expand/collapse button, not just the circled chevron.) - The modal traps focus while it's up. - Hitting Escape while the modal is up will dismiss it. - When the modal closes, focus returns to the chip button that caused it to open. - Screenreader experience: I think it's OK? I am not an expert screen reader user by any means. It's possible a few more things could use ARIA roles. One judgment call I made is to aria-label the question icon in the chips as "show glossary". When a chip is focused, the reader says "Tax credit show glossary, button". It seemed good to have a clue as to what will happen if you click the button? ### Next steps If this all seems suitable, I'll request translations. ## Test Plan - Look at the component on all widths, in several browsers (tried Chrome, Firefox, Safari, MobileSafari). Make sure the section corresponding to the incentive type you clicked on is open as soon as the modal appears, and no others are open. Click on others to open and close. Open all the sections and make sure the scrolling is correct. - Make sure the modal is dismissible by clicking away from the body, clicking the X button, or hitting Escape. - Scroll-wheel with the cursor away from the modal body, and make sure the document doesn't scroll underneath. - Navigate with keyboard: tab to the incentive-type chip and hit Enter. Make sure focus is immediately on the right section header. Tab and shift-tab and make sure focus is trapped within the modal. Hit Esc, or "enter" on the X button, and make sure focus is back on the original chip. - Do all of the above with VoiceOver on.

oyamauchi

Translations are requested, and I'll make a few updates from the feedback.

oyamauchi · 2024-09-09T22:27:07Z

src/glossary.tsx

+          <CircledChevron
+            w={40}
+            h={40}
+            fillClass="fill-[#ebebeb] group-data-open:fill-color-action-primary"


Sure -- I just took ebebeb from the designs; I don't know why Emiola went outside the design system colors though.

oyamauchi · 2024-09-09T22:28:18Z

src/glossary.tsx

+          // Prevent clicks on the body from closing the modal
+          onClick={e => e.stopPropagation()}


I meant the body of the modal itself -- the white-background div. Only clicks on the blurry background should close the modal. I can clarify that.

oyamauchi · 2024-09-09T22:29:35Z

src/state-incentive-details.tsx

+          aria-label={msg('Show glossary')}
+          w={10}
+          h={10}
+          opacity={1.0}


It's because the same QuestionIcon component is used for the tooltip icons in the form, where it has to be 50% opacity. I guess I should make it so 100% is the default, though, and the usage in the form has to specify 50% explicitly. I'll change that.

oyamauchi · 2024-09-09T22:33:21Z

src/state-incentive-details.tsx

+                <button
+                  type="button"


There's no Button component in this codebase -- it doesn't use a library of styled components at all. There are a couple of reusable things in src/buttons.tsx, but none of those are applicable here.

Also, the default <button> type is submit, which is not what we want here. (Although this button isn't within a form, so submit and button would have the same behavior anyway, but I'm just in the habit of writing type=button.)

vercel bot deployed to Preview July 22, 2024 22:00 View deployment

oyamauchi requested review from matelau and veekas July 22, 2024 22:03

matelau reviewed Jul 23, 2024

View reviewed changes

veekas approved these changes Jul 25, 2024

View reviewed changes

oyamauchi force-pushed the owen.glossary branch from 1bc75c6 to c84a7c5 Compare September 9, 2024 22:07

vercel bot deployed to Preview September 9, 2024 22:08 View deployment

oyamauchi commented Sep 9, 2024

View reviewed changes

changes from review feedback

6d7b28b

vercel bot deployed to Preview September 9, 2024 22:37 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add modal glossary of payment methods #181

Add modal glossary of payment methods #181

oyamauchi commented Jul 22, 2024 •

edited

Loading

vercel bot commented Jul 22, 2024 •

edited

Loading

matelau Jul 23, 2024 •

edited

Loading

oyamauchi Jul 25, 2024

veekas left a comment

veekas Jul 25, 2024

oyamauchi Sep 9, 2024

veekas Jul 25, 2024

oyamauchi Sep 9, 2024

veekas Jul 25, 2024

oyamauchi Sep 9, 2024

veekas Jul 25, 2024

oyamauchi Sep 9, 2024

oyamauchi left a comment

oyamauchi Sep 9, 2024

oyamauchi Sep 9, 2024

oyamauchi Sep 9, 2024

oyamauchi Sep 9, 2024

		// Prevent clicks on the body from closing the modal
		onClick={e => e.stopPropagation()}

Add modal glossary of payment methods #181

Are you sure you want to change the base?

Add modal glossary of payment methods #181

Conversation

oyamauchi commented Jul 22, 2024 • edited Loading

Description

Accessibility

Next steps

Test Plan

vercel bot commented Jul 22, 2024 • edited Loading

matelau Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oyamauchi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oyamauchi commented Jul 22, 2024 •

edited

Loading

vercel bot commented Jul 22, 2024 •

edited

Loading

matelau Jul 23, 2024 •

edited

Loading