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

[terra-tag] Create tag-list subcomponent #3854

Merged
merged 80 commits into from
Nov 7, 2023

Conversation

swetasingh9714
Copy link
Contributor

@swetasingh9714 swetasingh9714 commented Jul 21, 2023

Summary

Created a new component TagList which will have tags grouped together. A Subcomponent, Tag, was created for this main component.

What was changed: Created a new component terra-tag-list.

Why it was changed: To be able to group all tags together and add keyboard navigation using left and right arrow keys to navigate the list.

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


Thank you for contributing to Terra.
@cerner/terra

@swetasingh9714 swetasingh9714 requested a review from a team as a code owner July 21, 2023 12:03
@github-actions github-actions bot temporarily deployed to preview-pr-3854 July 21, 2023 12:03 Destroyed
@github-actions github-actions bot temporarily deployed to preview-pr-3854 July 21, 2023 12:22 Destroyed
@JessieRandle
Copy link
Contributor

Why was a new tag component created instead of using the existing terra-tag package?

@swetasingh9714
Copy link
Contributor Author

Why was a new tag component created instead of using the existing terra-tag package?

I created this PR only so I could get the functionality reviewed. I am still unclear whether a new package needs to be created in which case we would keep both terra-tag and terra-tag-list like it's done for terra-button or we need to have just one package like the one we have for terra-pills. If it's the former we can keep this structure else we can leverage the existing terra-tag package and update it.

@supreethmr supreethmr marked this pull request as draft July 24, 2023 17:50
@supreethmr
Copy link
Contributor

Why was a new tag component created instead of using the existing terra-tag package?

I created this PR only so I could get the functionality reviewed. I am still unclear whether a new package needs to be created in which case we would keep both terra-tag and terra-tag-list like it's done for terra-button or we need to have just one package like the one we have for terra-pills. If it's the former we can keep this structure else we can leverage the existing terra-tag package and update it.

I think design was to create a new subcomponent ( basically container which would have prop children of type terra-tag ) within terra-tag component to support list of terra-tag components. Can you revert these changes and add new subcomponent for tag-list like terra-dropdown-button-split-button.

@github-actions github-actions bot temporarily deployed to preview-pr-3854 July 25, 2023 11:59 Destroyed
@github-actions github-actions bot temporarily deployed to preview-pr-3854 July 25, 2023 12:54 Destroyed
@saket2403 saket2403 changed the title New: Create new component - 'terra-tag-list' [terra-tag] Create tag-list subcomponent Jul 26, 2023
@github-actions github-actions bot temporarily deployed to preview-pr-3854 July 27, 2023 09:21 Destroyed
@github-actions github-actions bot temporarily deployed to preview-pr-3854 July 31, 2023 12:27 Destroyed
@github-actions github-actions bot temporarily deployed to preview-pr-3854 August 1, 2023 10:42 Destroyed
@MadanKumarGovindaswamy
Copy link
Contributor

It is based on the way the logic has been desired. This could be handled in React but would require architectural design changes. I can review it in its current state due to time constraints, but I am not sure this is the correct design.

@cm9361 We have addressed the PR review comments can you have a look into that.

We updated Terra-TagList to use state variables to update Tabindex and we found that there was not much difference on page load time compared to the existing way of updating the tabIndex instead this approach adding state requires more code changes to the existing implementation of TagList.

Adding snapshots of performance loading:

Below screenshot was taken while performing keyboard navigation which updates tabIndex through setAttribute method

Navigation without state with multiple tags

Below screenshot was taken while performing keyboard navigation which updates tabIndex through state variable
Navigation after state with multiple tags

The existing approach has been validated with multiple components and scenarios and works as expected.
New implementation requires some of the validations to be performed again which conflicts with the current time constraints.

Thanks

@MadanKumarGovindaswamy
Copy link
Contributor

Hi @scottwilmarth . Could you please review this PR. It's been pending for the UXApproval. Thanks

@supreethmr

This comment was marked as resolved.

@MadanKumarGovindaswamy

This comment was marked as resolved.

@github-actions github-actions bot temporarily deployed to preview-pr-3854 November 6, 2023 13:41 Destroyed
@rahulkumar8cs
Copy link

+1 for accessibility testing and Approved
Validated with Windows 11 cloud desktop, Edge Version 116.0.1938.62 (Official build) (64-bit), JAWS 2023.2307.37
When VPC cursor is on:
When user navigates to the "11 more" button, then the hidden list items appears on the page. Then when user further navigates to the "Show Less" button and trigger it, then list items are collapsed and the button instruction "11 items are hidden, select to display hidden items" is exposed twice by JAWS.

@supreethmr supreethmr added ⭐ UX Reviewed UX Reviewed and approved. and removed UX Review Required labels Nov 7, 2023
@supreethmr supreethmr merged commit 6726ff4 into main Nov 7, 2023
22 checks passed
@MadanKumarGovindaswamy
Copy link
Contributor

Created a jira for this issue found in accessibility testing.
JIRA Link: https://jira2.cerner.com/browse/UXPLATFORM-9812
Thanks

@supreethmr supreethmr deleted the terra-tag_keyboard-navigation branch November 7, 2023 11:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
📦 terra-tag ⭐ Accessibility Reviewed Accessibility has been reviewed and approved. ⭐ UX Reviewed UX Reviewed and approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants