department-of-veterans-affairs / va-mobile-app

"If VA were a company, it would have a flagship mobile app."
https://department-of-veterans-affairs.github.io/va-mobile-app/
15 stars 2 forks source link

CU - Remove testID from VABulletList component #5779

Open alexandec opened 1 year ago

alexandec commented 1 year ago

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

timwright12 commented 9 months ago

@alexandec do we still need this ticket?

alexandec commented 9 months ago

@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.

juancstlm-a6 commented 4 days ago

I believe this to have been completed in https://github.com/department-of-veterans-affairs/va-mobile-app/commit/d5721ee63a425662d8be80b518f085994689d97f#diff-580f6e35c23c01211dbeffcd729de2a8cb34ec1eaec152aef1a6c67474b5d84dL98

TKDickson commented 3 days ago

@alexandec for ^

alexandec commented 3 days ago

@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 commented 3 days ago

@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 commented 3 days ago

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

juancstlm-a6 commented 1 day ago

@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 commented 1 day ago

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'),
}