Skip to content

feat(ActionButton): use button component for action button#1358

Open
ethanashaw wants to merge 1 commit intomainfrom
use-button-for-action-button
Open

feat(ActionButton): use button component for action button#1358
ethanashaw wants to merge 1 commit intomainfrom
use-button-for-action-button

Conversation

@ethanashaw
Copy link
Copy Markdown
Contributor

Done

I changed ActionButton to use the Button component instead of the default HTML button, allowing for the extra props.

I made a change to one of the ConfirmationModal stories showing this off. We needed to include an icon for a confirmation button, but the button couldn't be styled with the hasIcon prop before.

QA

Pinging @canonical/react-library-maintainers for a review.

Storybook

To see rendered examples of all react-components, run:

yarn start

QA in your project

from react-components run:

yarn build
npm pack

Install the resulting tarball in your project with:

yarn add <path-to-tarball>

QA steps

  • Steps for QA.

Percy steps

  • No visual changes expected.

Co-authored-by: Copilot <copilot@github.com>
Copilot AI review requested due to automatic review settings April 24, 2026 23:40
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates ActionButton to render the shared Button component so it can accept additional Button props (e.g. hasIcon), and demonstrates this in Storybook via ConfirmationModal.

Changes:

  • Switch ActionButton from a native <button> to the shared <Button> component and widen its spread props to ButtonProps.
  • Attempt to add ref support to Button via a ref prop.
  • Update ConfirmationModal Storybook story to show an icon + label confirm button using hasIcon.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/components/ActionButton/ActionButton.tsx Renders Button instead of native button and expands accepted props to ButtonProps.
src/components/Button/Button.tsx Adds a ref field to the exported prop type in an attempt to support refs.
src/components/ConfirmationModal/ConfirmationModal.stories.tsx Demonstrates icon+label confirm button using confirmButtonProps.hasIcon.
Comments suppressed due to low confidence (1)

src/components/ActionButton/ActionButton.tsx:180

  • ActionButton relies on ref.current to measure the button size, but passing ref={ref} to <Button /> will not attach the ref unless Button forwards refs. As-is, ref.current will stay null and the loader sizing logic won’t run. Either switch Button to forwardRef (preferred) or revert this component to rendering a native <button> and manually apply the needed Button classes/props.
    <Button
      className={buttonClasses}
      ref={ref}
      onClick={isDisabled ? onClickDisabled : onClick}
      aria-disabled={isDisabled || undefined}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +68 to 72
/**
* A ref to the button.
*/
ref?: Ref<HTMLButtonElement>;
} & (Omit<ButtonHTMLAttributes<HTMLButtonElement>, "onClick"> | P);
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

Adding a ref prop to Button doesn’t actually make <Button ref={...} /> work: React treats ref as a special attribute, so it won’t be delivered via props to this function component (and will emit a runtime warning). To support refs, convert Button to React.forwardRef and pass the forwarded ref to the underlying element (and remove ref from the regular props shape / use RefAttributes).

Copilot uses AI. Check for mistakes.
Comment on lines +176 to 179
<Button
className={buttonClasses}
ref={ref}
onClick={isDisabled ? onClickDisabled : onClick}
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

Now that ActionButton renders <Button>, the appearance ? \p-button--${appearance}` : "p-button"class inbuttonClassescombines withButton’s own default p-buttonclass (sinceappearanceisn’t passed through), changing the rendered class list vs before (e.g.p-button p-button--negative). Pass appearance={appearance}toButtonand remove thep-button...portion frombuttonClassesto keep styling consistent with theButton` component.

Copilot uses AI. Check for mistakes.
Comment on lines 171 to +176
// This component uses the base button element instead of the Button component
// as the button requires a ref and Button would have to be updated to use
// forwardRef which is not currently supported by components that use
// typescript generics.
return (
<button
<Button
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

The comment above the return still says this component uses a native button because Button doesn’t support refs, but the implementation now renders <Button>. Please update/remove the comment so it matches the new behavior (and the chosen ref strategy).

Copilot uses AI. Check for mistakes.
import Icon from "components/Icon";

const doNothing = () => {};
const doNothing = () => { };
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

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

Formatting: the codebase’s stories consistently use const doNothing = () => {}; (e.g. src/components/ConfirmationButton/ConfirmationButton.stories.tsx:8). Please remove the extra spaces (() => { };) to match existing conventions.

Suggested change
const doNothing = () => { };
const doNothing = () => {};

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants