[terra-button] Create selectable button #3940

sugan2416 · 2023-10-16T13:11:56Z

Summary

What was changed:

Add private prop isSelectable to allow terra-button to function as a toggleable button.

Why it was changed:

These changes were requested by Mpages team to have a sub-variant of terra-button which could be toggleable.

Testing

This change was tested using:

Reviews

In addition to engineering reviews, this PR needs:

UX review
Accessibility review
Functional review

Additional Details

This PR resolves:

UXPLATFORM-9749

Thank you for contributing to Terra.
@cerner/terra

Unselected

Selected

ShettyAkarsh · 2023-10-19T05:14:10Z

packages/terra-button/src/Button.jsx

+   * @private
+   * Whether or not the button can be selected (toggleable button).
+   */
+  isSelectable: PropTypes.bool,


can we add a console.warn when this prop s used ? something like 'this prop will be deprecated in future and do not recommend it for use'.

Added in 3ea7af9

Removed in 44abd39 as prop was changed to custom.

ShettyAkarsh · 2023-10-19T05:15:12Z

packages/terra-button/src/Button.jsx

@@ -326,6 +350,7 @@ class Button extends React.Component {
        onFocus={this.handleFocus}
        href={href}
        ref={refCallback}
+        aria-pressed={isSelectable ? this.state.isSelected : undefined}


what is the SR reading out when aria-pressed is set to True ?

It says "Toggle button pressed, <button_label>"

supreethmr · 2023-10-19T14:06:57Z

packages/terra-button-group/tests/jest/__snapshots__/ButtonGroup.test.jsx.snap

@@ -46,7 +46,6 @@ exports[`correctly applies the theme context className 1`] = `
        >
          <button
            aria-disabled={false}
-            aria-pressed={false}


why are we removing aria-pressed from button group ..?

This was getting removed due to the condition in button. Updated button component to add aria-pressed only when "isSelected" is used so that ButtonGroup would continue to receive aria-pressed as before.

aria-pressed is now back in 54c4c36

supreethmr · 2023-10-19T14:08:47Z

packages/terra-button/src/Button.jsx

@@ -154,6 +163,8 @@ class Button extends React.Component {
      this.shouldShowFocus = true;
    }

+    this.handleOnChange(event);


let's invoke this only for selectable button

check was added in within handleOnChange (54c4c36#diff-39f6c87ccf92298263c905de24e815f96ac1c87e46309e7d88ac4513f8bb7193R152).

Now moved before calling handler in 54c4c36

supreethmr · 2023-10-19T14:11:11Z

packages/terra-button/src/Button.jsx

@@ -174,7 +185,7 @@ class Button extends React.Component {

    // Add focus styles for keyboard navigation
    if (event.nativeEvent.keyCode === KeyCode.KEY_SPACE || event.nativeEvent.keyCode === KeyCode.KEY_RETURN) {
-      this.setState({ focused: true });
+      this.setState(prevState => ({ focused: true, isSelected: !prevState.isSelected }));


I would move this out to different block and update only if isSelectable is true to prevent unintended re-render of component

Added check in 54c4c36

supreethmr · 2023-10-19T14:11:50Z

packages/terra-button/src/Button.jsx

@@ -212,6 +223,7 @@ class Button extends React.Component {
    if (this.props.onMouseDown) {
      this.props.onMouseDown(event);
    }
+    this.setState(prevState => ({ isSelected: !prevState.isSelected }));


same here we need to invoke only for selectable button

Added check in 54c4c36

supreethmr · 2023-10-19T14:13:02Z

packages/terra-button/src/Button.jsx

@@ -255,6 +269,7 @@ class Button extends React.Component {
        { compact: isCompact },
        { 'is-active': this.state.active && !isDisabled },
        { 'is-focused': this.state.focused && !isDisabled },
+        { 'is-selected': customProps.isSelectable && this.state.isSelected && !isDisabled },


let's spread out the isSelectable on top and use it instead of referring it through customProps chaining on multiple place for better readability

That would cause unlisted prop lint err. Instead now moved to the top like this: 54c4c36#diff-39f6c87ccf92298263c905de24e815f96ac1c87e46309e7d88ac4513f8bb7193R132

This way handlers can use it outside of render.

supreethmr · 2023-10-19T14:16:56Z

packages/terra-button/src/Button.module.scss

+    &.is-selected {
+      background-color: var(--terra-button-selected-background-color, #52585b);
+      border-color: var(--terra-button-selected-border-color, #52585b);
+      box-shadow: var(--terra-button-selected-box-shadow);
+      color: var(--terra-button-selected-color, #fff);
+    }


I believe is-Selected is the only class we would need rest other styles should be same as default button ( which might be already added on _mixin.scss of button package.

Updated along with renaming in cfcc950

sugan2416 · 2023-10-20T12:24:22Z

Unlisted props seem to cause lint err in ComponenetDidMount as well.
Removed on change handler
Added wdio test for selected.
Updated and renamed style var

sugan2416 added 📦 terra-button Accessibility Review Required labels Oct 16, 2023

sugan2416 self-assigned this Oct 16, 2023

github-actions bot temporarily deployed to preview-pr-3940 October 16, 2023 13:12 Destroyed

github-actions bot temporarily deployed to preview-pr-3940 October 17, 2023 14:02 Destroyed

sugan2416 marked this pull request as ready for review October 18, 2023 07:14

sugan2416 requested a review from a team as a code owner October 18, 2023 07:14

sugan2416 requested review from supreethmr and saket2403 October 18, 2023 07:14

sugan2416 force-pushed the isselected-button branch from 6ecd187 to 4aeef39 Compare October 18, 2023 07:25

github-actions bot temporarily deployed to preview-pr-3940 October 18, 2023 07:32 Destroyed

ShettyAkarsh reviewed Oct 19, 2023

View reviewed changes

github-actions bot temporarily deployed to preview-pr-3940 October 19, 2023 07:24 Destroyed

github-actions bot temporarily deployed to preview-pr-3940 October 19, 2023 11:33 Destroyed

sugan2416 force-pushed the isselected-button branch from cc612cd to 80c275b Compare October 19, 2023 11:39

github-actions bot temporarily deployed to preview-pr-3940 October 19, 2023 11:42 Destroyed

github-actions bot temporarily deployed to preview-pr-3940 October 19, 2023 11:53 Destroyed

supreethmr reviewed Oct 19, 2023

View reviewed changes

github-actions bot temporarily deployed to preview-pr-3940 October 20, 2023 10:21 Destroyed

github-actions bot temporarily deployed to preview-pr-3940 October 20, 2023 12:22 Destroyed

cerner deleted a comment from github-actions bot Oct 20, 2023

Sugan G added 2 commits October 20, 2023 17:56

selectable button

bd115ec

added aria-pressed

d58f236

Sugan G added 6 commits October 20, 2023 17:56

updated changelog

066a5da

added warn

57e7e3c

changed to custom prop

c7463f7

updated changelog

2eb64c1

review updates

b26aec3

review updates, wdio

e23b8e0

sugan2416 force-pushed the isselected-button branch from cfcc950 to e23b8e0 Compare October 20, 2023 12:27

github-actions bot temporarily deployed to preview-pr-3940 October 20, 2023 12:31 Destroyed

code cleanup

d2bb547

github-actions bot deployed to preview-pr-3940 October 23, 2023 06:17 View deployment

supreethmr approved these changes Oct 23, 2023

View reviewed changes

supreethmr removed the Accessibility Review Required label Oct 23, 2023

supreethmr merged commit 6b6dbdf into main Oct 23, 2023
21 checks passed

supreethmr deleted the isselected-button branch October 23, 2023 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[terra-button] Create selectable button #3940

[terra-button] Create selectable button #3940

sugan2416 commented Oct 16, 2023 •

edited

Loading

ShettyAkarsh Oct 19, 2023

sugan2416 Oct 19, 2023

sugan2416 Oct 19, 2023

ShettyAkarsh Oct 19, 2023

sugan2416 Oct 19, 2023

supreethmr Oct 19, 2023

sugan2416 Oct 20, 2023

supreethmr Oct 19, 2023

sugan2416 Oct 20, 2023

supreethmr Oct 19, 2023

sugan2416 Oct 20, 2023

supreethmr Oct 19, 2023

sugan2416 Oct 20, 2023

supreethmr Oct 19, 2023 •

edited

Loading

sugan2416 Oct 20, 2023

supreethmr Oct 19, 2023

sugan2416 Oct 20, 2023

sugan2416 commented Oct 20, 2023 •

edited

Loading

[terra-button] Create selectable button #3940

[terra-button] Create selectable button #3940

Conversation

sugan2416 commented Oct 16, 2023 • edited Loading

Summary

Testing

Reviews

Additional Details

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

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

supreethmr Oct 19, 2023 • edited Loading

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

sugan2416 commented Oct 20, 2023 • edited Loading

sugan2416 commented Oct 16, 2023 •

edited

Loading

supreethmr Oct 19, 2023 •

edited

Loading

sugan2416 commented Oct 20, 2023 •

edited

Loading