Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: Add Action Group component, e.g. to wrap Dialog buttons in #1592

Merged
merged 22 commits into from
Oct 1, 2024

Conversation

VincentSmedinga
Copy link
Contributor

@VincentSmedinga VincentSmedinga commented Sep 18, 2024

Describe the pull request

Thank you for contributing to the project!
Please use this template to help us handle your PR smoothly.

What

Introduces a Dialog.ActionGroup subcomponent.

Why

  • Encapsulates the responsibility of laying out a row of buttons (gap, wrapping, stretching) to a Dialog.ActionGroup subcomponent. We may extend it to a generic <ActionGroup> component later to follow NL Design System.
  • Allows placing the buttons within a local form element.
  • I expected we could lose the footer entirely, but that would remove whitespace between the dialog content and the buttons. Having the Action Group in a tall form hides them from view until the user scrolls. Consequently, the recommended approach is placing the Action Group in the footer and linking the submit button to the form manually. People can still move the Action Group into a form, but then they must implement whitespace and overflow themselves. I’ve documented this.

How

  • Created the subcomponent and moved styles from .ams-dialog-footer to .ams-dialog__action-group.
  • Improved documentation: reordered stories, reworded the information on forms, and reused some aspects of them.

Checklist

Before submitting your pull request, please ensure you have done the following. Check each checkmark if you have done so or if it wasn't necessary:

  • Add or update unit tests
  • Add or update documentation
  • Add or update stories
  • Add or update exports in index.* files
  • Start the PR title with a Conventional Commit prefix, as explained here.

Additional notes

  • None

@github-actions github-actions bot temporarily deployed to demo-DES-961-dialog-action-group September 19, 2024 13:32 Destroyed
@VincentSmedinga VincentSmedinga marked this pull request as ready for review September 19, 2024 13:58
@github-actions github-actions bot temporarily deployed to demo-DES-961-dialog-action-group September 19, 2024 14:05 Destroyed
@VincentSmedinga VincentSmedinga changed the title feat!: Wrap Dialog buttons in an Action Group feat!: Wrap Dialog buttons in a new Action Group Sep 19, 2024
Copy link
Contributor

@dlnr dlnr left a comment

Choose a reason for hiding this comment

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

You're renaming stuff to action group but you leave the footer prop, probably because you still need the footer to stick to the bottom, and you move the footers flex properties to the action group. Why is an action group not a reusable component in other forms like NLDS button group? I thought this was the plan. This way it also counts as an "unnecessary wrapper" if you ask me.

@github-actions github-actions bot temporarily deployed to demo-DES-961-dialog-action-group September 20, 2024 10:13 Destroyed
@alimpens
Copy link
Contributor

Yeah, I agree with Niels, I don't think this is an improvement. Dialog.ActionGroup should render the footer imo (which would probably make it Dialog.Footer). If it doesn't, this doesn't really add anything to the old approach imo.

@VincentSmedinga
Copy link
Contributor Author

VincentSmedinga commented Sep 25, 2024

Well, that’s a surprise – this is precisely what we discussed. Except that we may not have realised that we couldn’t give up the entire footer concept.

You're renaming stuff to action group but you leave the footer prop, probably because you still need the footer to stick to the bottom, and you move the footers flex properties to the action group. Why is an action group not a reusable component in other forms like NLDS button group? I thought this was the plan. This way it also counts as an "unnecessary wrapper" if you ask me.

Correct. This separates the ‘footer’ responsibilities (sticking at the bottom and having whitespace between the body and itself) from the ‘buttons layout’ responsibilities (whitespace between them, wrapping, stacking). This is component-driven design. And yes, that introduces one extra div, but with good reason. As I mentioned in the description, the next step can be to make this an independent component – we could even do that immediately, as far as I’m concerned.

Yeah, I agree with Niels, I don't think this is an improvement. Dialog.ActionGroup should render the footer imo (which would probably make it Dialog.Footer). If it doesn't, this doesn't really add anything to the old approach IMO.

It adds that people can use the Action Group either in the footer or the body, depending on their needs. And after generalizing the component, we can use it everywhere to lay out multiple buttons.

I think we should still go this route.

@github-actions github-actions bot temporarily deployed to demo-DES-961-dialog-action-group September 25, 2024 16:16 Destroyed
@dlnr
Copy link
Contributor

dlnr commented Sep 26, 2024

I vote for a Button Group component like NLDS: https://nl-design-system.github.io/utrecht/storybook/?path=/docs/css_css-button-group--docs

Having a dialog footer and actiongroup is confusing, also because you can only use the actiongroup in the footer prop.

This would also benefit the form examples we are doing, and I've had the need for a component like this in Figma on more than one occasion.

@VincentSmedinga
Copy link
Contributor Author

I vote for a Button Group component like NLDS

Hah, that’s precisely what this is! Except that Action Group is the new name for Button Group, and I suggested promoting it to an independent component later.

We discussed this offline and agreed that the approach in this PR is correct. We’re even making it a separate component directly.

@github-actions github-actions bot temporarily deployed to demo-DES-961-dialog-action-group September 26, 2024 11:52 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-961-dialog-action-group September 26, 2024 13:18 Destroyed
@VincentSmedinga VincentSmedinga changed the title feat!: Wrap Dialog buttons in a new Action Group feat!: Add Action Group component, e.g. to wrap Dialog buttons in Sep 26, 2024
@github-actions github-actions bot temporarily deployed to demo-DES-961-dialog-action-group September 26, 2024 13:35 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-961-dialog-action-group September 26, 2024 14:00 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-961-dialog-action-group September 26, 2024 14:14 Destroyed
storybook/src/components/Dialog/Dialog.docs.mdx Outdated Show resolved Hide resolved
packages/css/src/components/dialog/README.md Outdated Show resolved Hide resolved
packages/css/src/components/link/README.md Outdated Show resolved Hide resolved
packages/react/src/ActionGroup/ActionGroup.tsx Outdated Show resolved Hide resolved
packages/react/src/ActionGroup/ActionGroup.test.tsx Outdated Show resolved Hide resolved
storybook/src/styles/docs.css Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to demo-DES-961-dialog-action-group September 27, 2024 13:43 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-961-dialog-action-group September 27, 2024 15:39 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-961-dialog-action-group September 30, 2024 14:06 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-961-dialog-action-group October 1, 2024 10:58 Destroyed
@VincentSmedinga VincentSmedinga merged commit d0ea054 into develop Oct 1, 2024
6 checks passed
@VincentSmedinga VincentSmedinga deleted the feature/DES-961-dialog-action-group branch October 1, 2024 11:25
@github-actions github-actions bot mentioned this pull request Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants