components icon indicating copy to clipboard operation
components copied to clipboard

feat: Add support for undismissible tokens in token group

Open timogasda opened this issue 1 year ago • 1 comments

Description

This change adds support for undismissible tokens in Token Group. When no onDismiss event handler is provided, the tokens will no longer render a dismiss button.

Related links, issue #, if available: AWSUI-24723

Screenshot of regular and non-dismissible token groups

Preview: https://d21d5uik3ws71m.cloudfront.net/components/393ba3296dcc838fd33263b240453f3906021b53/dev-pages/index.html#/light/token-group/simple

Using the onDismiss event vs. adding a new dismissible property

I've decided to use the existence of the onDismiss function as an indicator for displaying or hiding the dismiss icons. The mean reason for this is that it requires no changes to the component interface. This is not the first time we've used this pattern in the system. Autosuggest determines which loading mechanism to use by checking the existence of a onLoadItems callback. The internal Token component also uses this behavior already: https://github.com/cloudscape-design/components/blob/30552b551ca3f17f8523104a6d67a156356a45a2/src/token-group/token.tsx#L53.

Alternatively we could add a new property dismissible to control this behavior, but I currently don't see a reason to make that addition to the API.

State management changes

This change means that the Token group components changes from being controlled to being controllable, depending on whether or not the tokens can be dismissed. This will be reflected in the development guidelines.

Note that this would happen even if we decided to go with the new dismissible property approach.

How has this been tested?

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

timogasda avatar Feb 07 '24 11:02 timogasda

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (2878ee9) 94.78% compared to head (bab1bd6) 94.79%. Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1940   +/-   ##
=======================================
  Coverage   94.78%   94.79%           
=======================================
  Files         662      662           
  Lines       17932    17936    +4     
  Branches     5923     5927    +4     
=======================================
+ Hits        16997    17002    +5     
+ Misses        870      869    -1     
  Partials       65       65           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 07 '24 11:02 codecov[bot]