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

CU - Remove testID from VABulletList component #5779

Closed
alexandec opened this issue May 23, 2023 · 16 comments · Fixed by #9736
Closed

CU - Remove testID from VABulletList component #5779

alexandec opened this issue May 23, 2023 · 16 comments · Fixed by #9736
Assignees
Labels
code upkeep front-end Ticket requires front-end work global Issues for the global team Good first issue An issue that's suitable for someone looking to contribute for the first time

Comments

@alexandec
Copy link
Contributor

alexandec commented May 23, 2023

Proposed Change

The VABulletList component uses a testID which is not needed. The testID necessitates an extra workaround when using the boldedTextPrefix and boldedText props because the testID omits those props, so they aren't read by the screen reader. This ticket covers:

  1. Removing the testID from the VABulletList component
  2. Ensuring the a11yLabel prop still works correctly with testID gone
  3. Auditing existing usage of VABulletList and removing unneeded a11yLabels which were only added as workarounds (see above)

Why Should We Prioritize?

Engineers will assume VABulletList announces bolded text, but due to the testID issue, it does not. This could cause a11y issues. In addition, adding labels as workarounds takes extra time and is error-prone. Removing the testID will fix these issues.

Coding Time Estimation

3

Testing Considerations

All uses of VABulletList will need to be checked with screen readers on iOS and Android. There are 17 files using VABulletList as of May 2023.

Checklist

[X] Add the front-end label tag

[X] Attach to the Frontend epic for engineering initiatives

@alexandec alexandec added code upkeep front-end Ticket requires front-end work labels May 23, 2023
@timwright12
Copy link
Contributor

@alexandec do we still need this ticket?

@alexandec
Copy link
Contributor Author

@timwright12 yes, this ticket is still valid. To see examples of the issue, search for "boldedText" in the app code. We still have 6 components where the issue occurs.

@theodur theodur added the global Issues for the global team label Mar 11, 2024
@timwright12 timwright12 added the Good first issue An issue that's suitable for someone looking to contribute for the first time label Aug 7, 2024
@juancstlm-a6 juancstlm-a6 self-assigned this Sep 24, 2024
@juancstlm-a6
Copy link
Contributor

I believe this to have been completed in d5721ee#diff-580f6e35c23c01211dbeffcd729de2a8cb34ec1eaec152aef1a6c67474b5d84dL98

@TKDickson
Copy link
Contributor

@alexandec for ^

@alexandec
Copy link
Contributor Author

@juancstlm-a6 @TKDickson it looks like points 1 and 2 are completed already, but point 3, auditing existing usage of VABulletList and removing unneeded a11yLabels which were only added as workarounds, is not. Searching the repo for "boldedText" still shows examples of VABulletList usage where there are a11yLabels which should no longer be needed.

@juancstlm-a6
Copy link
Contributor

@alexandec thanks for the clarification.

I'll take a look at point 3 as 1 and 2 have been completed

  1. Auditing existing usage of VABulletList and removing unneeded a11yLabels which were only added as workarounds (see above)

@TKDickson
Copy link
Contributor

@juancstlm-a6 FYI about the existence of this PR (should be merging into develop sometime today) as you start an audit

@TKDickson TKDickson self-assigned this Sep 27, 2024
@juancstlm-a6
Copy link
Contributor

juancstlm-a6 commented Sep 27, 2024

@alexandec For clarification the VABulletList should announce boldedText and boldedTextPrefix without the need for a a11yLabel
It is kind of implied that by completing Point 1 this will enable the aforementioned feature.

@juancstlm-a6
Copy link
Contributor

For Items that include a boldedText and a a11yLabel with a helper a11yLabelVA() helper. Ex: Below

{
  text: t('prescription.details.banner.bullet1'),
  boldedText: ' ' + t('or'),
  a11yLabel: a11yLabelVA(t('prescription.details.banner.bullet1')) + ' ' + t('or'),
}

I will be leaving those alone as is and not removing the a11yLabel since the a11yLabelVA function replaces the "VA" into "V-A" for accessibility.

We could, and I would be glad to, create a separate ticket where we can modify the accessibilityLabel of VABulletList to automagically apply this helper function like accessibilityLabel={a11yLabelVA(a11yLabel || '')} so that we can further reduce the need for a a11yLabel and save time and reduce potential mistakes where a translation string did not get wrapped with the helper when it should have. This would in turn allow us to remove the a11yLabel from the example above and just have the following:

{
  text: t('prescription.details.banner.bullet1'),
  boldedText: ' ' + t('or'),
}

@juancstlm-a6 juancstlm-a6 linked a pull request Sep 27, 2024 that will close this issue
12 tasks
@alexandec
Copy link
Contributor Author

@juancstlm-a6 thanks for working on this, and I agree that the a11yLabelVA function is still needed in some cases. I checked out your draft PR and left a comment there about a possible method of simplification. See what you think.

@TKDickson TKDickson added the blocked-QA Ticket is blocked by QA label Oct 3, 2024
@TKDickson
Copy link
Contributor

Over my ticket capacity, so not working on this currently. Adding blocked label.

@TKDickson TKDickson removed the blocked-QA Ticket is blocked by QA label Oct 4, 2024
@TKDickson
Copy link
Contributor

Picking this up now, removing blocked label

@TKDickson
Copy link
Contributor

TKDickson commented Oct 4, 2024

Checking written & announced content for correct & matching formatting (ex: bolding) on all screens with bolded text in VA Bullet lists:

  • LOA gate
  • Gender ID what to know
  • Reply help
  • RX details 'banner' (for transferred meds, on med details screen)
  • Appeal details for active appeal (for 'pending_form9', will need to mock, therefore iOS testing only)
  • Claim details for closed claim
  • Rx history 'not authorized' (Judy should have this)
  • Not enrolled in SM (can mock via authorized services, therefore iOS testing only)

@TKDickson
Copy link
Contributor

TKDickson commented Oct 4, 2024

First issue: #9801.
Found a terrible swipe order on the LOA gate screen, android only. It also is in develop so will be writing up a separate bug ticket for it. Appears to be OS/device specific, was only seeing on my Pixel on 14 (not OnePlus on 11)

Found a similar swipe order issue (will need to check later if also in develop) on the meds not authorized alert, the Rx transferred med details banner.... seems to be impacting all expand/collapse, or alerts, on this device (Android 14 would be my first guess). Will want to check against the implementation of the DS alert component before/after.

Second issue:
Found a link that's not announcing as a link, iOS 18 (could be more OS), also impacts develop & will be writing up separately: #9800.

@TKDickson
Copy link
Contributor

OK, tickets written for the bugs found during testing & not caused by this change, #9800 ad #9801 mentioned in prior comment.

The Android UI automation scripts in the PR are fine (just the one failure for URL links in the messages script that has also been failing in develop), but there was an app crash in Profile for iOS that I haven't seen before. Re-running the script to see if it was a one-off/flake, and if not I'll dig in to what's going on there.

@TKDickson
Copy link
Contributor

iOS passed on a re-run, this is good to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code upkeep front-end Ticket requires front-end work global Issues for the global team Good first issue An issue that's suitable for someone looking to contribute for the first time
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants