Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-date-time-picker] Fixed accessibility issue #2140

Merged
merged 6 commits into from
Apr 19, 2024

Conversation

adavijit
Copy link
Collaborator

@adavijit adavijit commented Apr 17, 2024

Summary

What was changed:

  • Added a div tag with role="group" to group the DatePicker and TimeInput component
  • Fixed wdio test

Why it was changed:
DatePicker and TimeInput component was not grouped together leading to accessibility issue

Testing

This change was tested using:

  • WDIO
  • Jest
  • Visual testing (please attach a screenshot or recording)
  • Other (please describe below)
  • No tests are needed

Reviews

In addition to engineering reviews, this PR needs:

  • UX review
  • Accessibility review
  • Functional review

Additional Details

This PR resolves:

UXPLATFORM-10229


Thank you for contributing to Terra.
@cerner/terra

@github-actions github-actions bot temporarily deployed to preview-pr-2140 April 17, 2024 10:58 Destroyed
@github-actions github-actions bot temporarily deployed to preview-pr-2140 April 17, 2024 12:02 Destroyed
@adavijit adavijit changed the title UXPLATFORM-10229_fix added div tag [terra-date-time-picker] Fixed accessibility issue Apr 17, 2024
@adavijit adavijit self-assigned this Apr 17, 2024
@github-actions github-actions bot temporarily deployed to preview-pr-2140 April 18, 2024 05:19 Destroyed
@adavijit adavijit requested a review from sugan2416 April 18, 2024 06:53
@rbsree
Copy link

rbsree commented Apr 18, 2024

+1 for accessibility review for Terra Date Time Picker, as suggested the Date and Time input field are grouped.

/>

{this.state.isAmbiguousTime && this.state.dateTime ? this.renderTimeClarification() : null}
<div role="group" aria-label={ariaLabel}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add another div? Can we add the role and aria-label to the existing <div className={cx('date-facade')}> div?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are correct! Added the same in the root div. Also added test for the same

@sycombs
Copy link
Contributor

sycombs commented Apr 18, 2024

Can we also add some Jest tests for the inclusion of the role and aria-label attributes?

@github-actions github-actions bot temporarily deployed to preview-pr-2140 April 19, 2024 07:46 Destroyed
@sugan2416 sugan2416 merged commit 1ded078 into main Apr 19, 2024
22 checks passed
@sugan2416 sugan2416 deleted the UXPLATFORM-10229_fix branch April 19, 2024 08:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants