feat: Support for toggle buttons in button group #2909

pan-kot · 2024-10-21T14:10:34Z

Description

The PR adds support for toggle buttons to the button group component.

Depends on:

fix: Fixes button group item states #2952

Rel: [IEbDAJOcw4NY], [a1MAAZpYYH9h]

How has this been tested?

New unit tests
New permutations and screenshot tests

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

If the code handles URLs: all URLs are validated through the checkSafeUrl function.

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.

codecov · 2024-10-21T14:19:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.25%. Comparing base (6ba69fa) to head (4f5350d).
Report is 11 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2909   +/-   ##
=======================================
  Coverage   96.25%   96.25%           
=======================================
  Files         769      769           
  Lines       21735    21780   +45     
  Branches     7431     7454   +23     
=======================================
+ Hits        20920    20964   +44     
- Misses        762      763    +1     
  Partials       53       53

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pan-kot · 2024-11-01T13:37:25Z

src/__tests__/__snapshots__/documenter.test.ts.snap

          {
            "name": "id",
            "optional": false,
            "type": "string",
          },
+          {
+            "name": "pressed",


This property is present when the new toggle button is clicked.

pan-kot · 2024-11-01T13:37:43Z

src/__tests__/__snapshots__/documenter.test.ts.snap

@@ -3790,11 +3790,21 @@ exports[`Documenter definition for button-group matches the snapshot: button-gro
      "detailInlineType": {
        "name": "InternalButtonGroupProps.ItemClickDetails",
        "properties": [
+          {
+            "name": "checked",


This property is present when a menu item of type "checkbox" is clicked.

pan-kot · 2024-11-01T13:39:43Z

src/button-group/__tests__/button-group-dev.test.tsx

@@ -0,0 +1,140 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.


I reworked button group unit tests. Now all tests are split across multiple files focusing on specific categories of tests. The tests ensure full coverage for all button group item types.

In the dev tests we validate dev warnings, fallbacks, and callbacks.

pan-kot · 2024-11-01T13:41:00Z

src/button-group/__tests__/button-group-focus.test.tsx

@@ -0,0 +1,66 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.


Focus tests validate the focus function works and the focus lands on the menu trigger after the menu is closed. The unit tests do not allow for some more complex checks such as focus delegation after an item is removed or becomes disabled - these transitions are covered with the integ tests.

pan-kot · 2024-11-01T13:42:12Z

src/button-group/__tests__/button-group-keyboard.test.tsx

@@ -0,0 +1,100 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.


The keyboard tests validate navigation and Escape that are handled explicitly. The keyboard tests do not include pressing buttons with Enter because that is the browser behaviour (the buttons do not define any onKeyDown handler, only the onClick). The behaviour is tested with integ tests instead.

pan-kot · 2024-11-01T13:43:25Z

src/button-group/__tests__/button-group.test.tsx

@@ -1,274 +0,0 @@
-// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.


All tests from this file are copied or re-implemented, no test scenario was excluded.

pan-kot · 2024-11-01T13:44:42Z

src/button-group/icon-toggle-button-item.tsx

+
+import testUtilStyles from './test-classes/styles.css.js';
+
+const IconToggleButtonItem = forwardRef(


This component is very similar to the existing IconButtonItem, see: https://github.com/cloudscape-design/components/blob/main/src/button-group/icon-button-item.tsx

pan-kot · 2024-11-01T13:45:42Z

src/button-group/menu-dropdown-item.tsx

@@ -26,7 +26,7 @@ const MenuDropdownItem = React.forwardRef(
  ) => {
    const containerRef = React.useRef<HTMLDivElement>(null);
    const onClickHandler = (event: CustomEvent<ButtonDropdownProps.ItemClickDetails>) => {
-      fireCancelableEvent(onItemClick, { id: event.detail.id }, event);
+      fireCancelableEvent(onItemClick, { id: event.detail.id, checked: event.detail.checked }, event);


Adding this to support selectable button dropdown items: https://cloudscape.design/components/button-dropdown?tabId=playground&example=with-selectable-items

pan-kot · 2024-11-01T13:46:36Z

src/toggle-button/internal.tsx

@@ -27,7 +27,7 @@ export const InternalToggleButton = React.forwardRef(
      onChange,
      className,
      ...rest
-    }: ToggleButtonProps,
+    }: ToggleButtonProps & { __title?: string },


This is needed to hide the HTML title of the button when it is used in the button group. That is because the title is not needed when the tooltip is present.

georgylobko

Good job! Just left a couple of minor comments on your discretion

georgylobko · 2024-11-01T14:50:58Z

src/button-group/__tests__/button-group-states.test.tsx

+  expect(wrapper.findToggleButtonById('copy')!.getElement()).toHaveAttribute('aria-disabled', 'true');
+  expect(wrapper.findMenuById('menu')!.findTriggerButton()!.getElement()).toHaveAttribute('aria-disabled', 'true');
+
+  await waitFor(() => expect(document.body).toHaveTextContent('Loading Like'));


I'm curious why this expectation checks the body content, not live regions' ?

That is because the new live region implementation: https://github.com/cloudscape-design/components/blob/main/src/live-region/controller.ts#L37

georgylobko · 2024-11-01T15:03:57Z

src/button-group/icon-toggle-button-item.tsx

+            trackRef={containerRef}
+            trackKey={item.id}
+            value={
+              (showFeedback && <InternalLiveRegion tagName="span">{feedbackContent}</InternalLiveRegion>) ||


All other components with a tooltip (but for disabled reason) use aria-describedby with a hidden element with the description, like in the button component https://github.com/cloudscape-design/components/blob/main/src/button/internal.tsx#L127

Should it be adopted here same way?

Great question!

The popover feedback is supposed to work the same as popover component: https://github.com/cloudscape-design/components/blob/main/src/popover/internal.tsx#L149

Ideally we should use the popover here but that causes a blink when the tooltips gets replaced with a popover.

pan-kot force-pushed the feat-button-group-toggle-buttons branch from 883bd49 to 1fe6263 Compare October 21, 2024 14:57

pan-kot deployed to dev-pages October 21, 2024 15:05 — with GitHub Actions View deployment

pan-kot force-pushed the feat-button-group-toggle-buttons branch 5 times, most recently from 16eb96c to 8f66e2d Compare October 28, 2024 12:36

pan-kot deployed to dev-pages October 28, 2024 12:43 — with GitHub Actions View deployment

pan-kot force-pushed the feat-button-group-toggle-buttons branch 2 times, most recently from 1d96218 to 665ddad Compare October 30, 2024 15:08

pan-kot deployed to dev-pages October 30, 2024 15:15 — with GitHub Actions View deployment

pan-kot force-pushed the feat-button-group-toggle-buttons branch from 665ddad to 20951d4 Compare October 30, 2024 15:17

pan-kot deployed to dev-pages October 30, 2024 15:23 — with GitHub Actions View deployment

pan-kot force-pushed the feat-button-group-toggle-buttons branch from 20951d4 to 421b593 Compare October 30, 2024 15:25

pan-kot deployed to dev-pages October 30, 2024 15:32 — with GitHub Actions View deployment

pan-kot force-pushed the feat-button-group-toggle-buttons branch 2 times, most recently from 2acd039 to 35cf88e Compare November 1, 2024 10:38

pan-kot deployed to dev-pages November 1, 2024 10:44 — with GitHub Actions View deployment

feat: Support for toggle buttons in button group

4f5350d

pan-kot commented Nov 1, 2024

View reviewed changes

pan-kot marked this pull request as ready for review November 1, 2024 13:46

pan-kot requested a review from a team as a code owner November 1, 2024 13:46

pan-kot requested review from at-susie and georgylobko and removed request for a team and at-susie November 1, 2024 13:46

pan-kot force-pushed the feat-button-group-toggle-buttons branch from 35cf88e to 4f5350d Compare November 1, 2024 13:49

pan-kot deployed to dev-pages November 1, 2024 13:56 — with GitHub Actions View deployment

georgylobko approved these changes Nov 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support for toggle buttons in button group #2909

feat: Support for toggle buttons in button group #2909

pan-kot commented Oct 21, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

pan-kot Nov 1, 2024

pan-kot Nov 1, 2024

pan-kot Nov 1, 2024

pan-kot Nov 1, 2024

pan-kot Nov 1, 2024

pan-kot Nov 1, 2024

pan-kot Nov 1, 2024

pan-kot Nov 1, 2024

pan-kot Nov 1, 2024

georgylobko left a comment

georgylobko Nov 1, 2024

pan-kot Nov 5, 2024

georgylobko Nov 1, 2024

pan-kot Nov 5, 2024

		@@ -0,0 +1,140 @@
		// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.

		@@ -0,0 +1,66 @@
		// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.

		@@ -0,0 +1,100 @@
		// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.

		@@ -1,274 +0,0 @@
		// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.


		import testUtilStyles from './test-classes/styles.css.js';

		const IconToggleButtonItem = forwardRef(

feat: Support for toggle buttons in button group #2909

Are you sure you want to change the base?

feat: Support for toggle buttons in button group #2909

Conversation

pan-kot commented Oct 21, 2024 • edited Loading

Description

How has this been tested?

Correctness

Security

Testing

codecov bot commented Oct 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgylobko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan-kot commented Oct 21, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading