Skip to content

bug(#2685): Move modal GoaButtonGroup actions#464

Merged
twjeffery merged 5 commits into
alphafrom
eric/2685-modal-buttongroup-actions
Dec 5, 2025
Merged

bug(#2685): Move modal GoaButtonGroup actions#464
twjeffery merged 5 commits into
alphafrom
eric/2685-modal-buttongroup-actions

Conversation

@willcodeforcoffee
Copy link
Copy Markdown
Collaborator

@willcodeforcoffee willcodeforcoffee commented Dec 1, 2025

This PR fixes GovAlta/ui-components#2685

It puts the ButtonGroup into the Modal#actions section properly.

Unfortunately the file had to get reformatted, which complicates reviewing it. I've separated the PR into two commits, the first reformats the file, the second has the change.

@willcodeforcoffee willcodeforcoffee self-assigned this Dec 1, 2025
@netlify
Copy link
Copy Markdown

netlify Bot commented Dec 1, 2025

Deploy Preview for abgov-ui-component-docs ready!

Name Link
🔨 Latest commit 3dc2f34
🔍 Latest deploy log https://app.netlify.com/projects/abgov-ui-component-docs/deploys/6930f36aa7043b0008882c9e
😎 Deploy Preview https://deploy-preview-464--abgov-ui-component-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Comment thread src/routes/components/Modal.tsx Outdated
{...componentProps}
open={open}
actions={
<GoabButtonGroup alignment="end" mt={"xl"}>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the 32px gap between modal content and actions is already built into the component, so it's not needed on the button group mt:

  • Content's last child gets margin-bottom: 16px
  • Actions container has padding-top: 16px

Adding mt="xl" (24px) to the GoabButtonGroup creates 56px total instead of 32px.

image image image image

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, @twjeffery . I removed the mt attribute completely.

@willcodeforcoffee willcodeforcoffee marked this pull request as ready for review December 2, 2025 16:05
Copy link
Copy Markdown
Collaborator

@twjeffery twjeffery left a comment

Choose a reason for hiding this comment

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

@willcodeforcoffee the examples look good now to me, thanks for updating.

One more thing I noticed, the modal component example in the sandbox on the first tab has actions, but the code snippet below shows no actions in a slot or otherwise (see code below):

<GoabButton onClick={onClick}>
  Show Modal
</GoabButton>
<GoabModal heading="Are you sure you want to exit your application?" open={open}>
  <p>
    Lorem ipsum dolor sit amet consectetur adipisicing elit. Mollitia obcaecati id molestiae, natus dicta, eaque qui iusto similique, libero explicabo eligendi eius laboriosam! Repellendus ducimus officia asperiores. Eos, eius numquam.
  </p>
</GoabModal>
<GoAButton onClick={onClick}>
  Show Modal
</GoAButton>
<GoAModal heading="Are you sure you want to exit your application?" open={open}>
  <p>
    Lorem ipsum dolor sit amet consectetur adipisicing elit. Mollitia obcaecati id molestiae, natus dicta, eaque qui iusto similique, libero explicabo eligendi eius laboriosam! Repellendus ducimus officia asperiores. Eos, eius numquam.
  </p>
</GoAModal>
Image Image

I'm not exactly sure how to change that generated code to properly show the actions within a slot like you've added, but @chrisolsen likely would

@willcodeforcoffee
Copy link
Copy Markdown
Collaborator Author

@willcodeforcoffee the examples look good now to me, thanks for updating.

One more thing I noticed, the modal component example in the sandbox on the first tab has actions, but the code snippet below shows no actions in a slot or otherwise (see code below):
[...]

@twjeffery You are right, and @chrisolsen does not believe we can do it with the way the code generation works on the front page. I tried a few different things but they didn't work. ☹️
In the examples for "Basic Modal" we do have a lot of examples using the actions though. I think they're different because they're inside CodeSandbox elements instead. I'm not sure what would be a good way to direct people to review those examples instead though... Maybe a link when "closable" is not checked? What would you think?

@willcodeforcoffee willcodeforcoffee marked this pull request as draft December 3, 2025 21:02
@willcodeforcoffee
Copy link
Copy Markdown
Collaborator Author

@twjeffery per our Slack discussion, I've removed the "Close" logic out of the Sandbox, and added a comment into the modal itself directing users to the Examples tab and learn more about using the actions for customization.

@willcodeforcoffee willcodeforcoffee marked this pull request as ready for review December 4, 2025 02:43
@twjeffery twjeffery merged commit e4e2caf into alpha Dec 5, 2025
7 checks passed
@twjeffery twjeffery deleted the eric/2685-modal-buttongroup-actions branch December 5, 2025 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Modal actions margins are causing padding issues

2 participants