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

[terra-folder-tree] Added tabIndex for only last focused item #1967

Merged
merged 6 commits into from
Jan 8, 2024

Conversation

artpark
Copy link
Contributor

@artpark artpark commented Jan 5, 2024

Summary

What was changed:

All folder tree children no longer has a tabIndex of 0. When moving through the elements with the arrow keys, the currently focused item is the only item with a tabIndex of 0.

Why it was changed:

Tabbing in and out of the component will not make the user tab through all the items to get to the intended item.

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-10109


Thank you for contributing to Terra.
@cerner/terra

Terra.validates.screenshot('tabbed out of focus', { selector: '#root' });

browser.keys(['Shift', 'Tab']);
Terra.validates.screenshot('level two folder focused keyboard', { selector: '#expand-collapse-folder-tree' });
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we're avoiding adding extra screenshots here, just wondering if there are any potential issues with validating the same screenshot twice in this test.

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've done this in the past before, and both screenshots should result in the same identical image, so I believe it should be alright

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a need to validate teh same screenshot twice? If we're only checking for focus, we can use the isFocused() validator since that's more efficient than doing a screenshot comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's right! I keep forgetting we have that, I'll update it to use that

@github-actions github-actions bot temporarily deployed to preview-pr-1967 January 5, 2024 17:16 Destroyed
@github-actions github-actions bot temporarily deployed to preview-pr-1967 January 5, 2024 20:09 Destroyed
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, +1!

@eawww eawww added ⭐ UX Reviewed UX Reviewed and approved. and removed UX Review Required labels Jan 5, 2024
@artpark artpark merged commit 4a74d50 into main Jan 8, 2024
22 checks passed
@artpark artpark deleted the folder-tree-tab branch January 8, 2024 14:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants