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

New button group component added #1484

Merged
merged 1 commit into from
May 22, 2021

Conversation

JonBunator
Copy link
Member

@JonBunator JonBunator commented May 2, 2021

A new button group component that groups related buttons together. As described in #1249
Tell me what you think :)

Edit:

  • RTL support added
  • Split button support added
  • Support for MudIconButton and MudToggleIconButton added

picture1

picture2

buttongroup
closes #1249

@porkopek
Copy link
Contributor

porkopek commented May 2, 2021

It looks cool!

@JonBunator JonBunator marked this pull request as draft May 6, 2021 09:41
@JonBunator JonBunator marked this pull request as ready for review May 6, 2021 18:54
@JonBunator
Copy link
Member Author

JonBunator commented May 13, 2021

This component could be used to create a split button. But it doesn't look good yet because MudButtons are too wide and the icon is not centered when they have no text.
grafik
Do you think it would be better to make changes to the MudButton class or should I use a MudIconButton and override its styles?

@JonBunator JonBunator marked this pull request as draft May 13, 2021 14:48
@mikes-gh
Copy link
Contributor

Do you think it would be better to make changes to the MudButton class or should I use a MudIconButton and override its styles?

@JonBunator would any public API change in either senario?

@JonBunator
Copy link
Member Author

JonBunator commented May 14, 2021

@mikes-gh

@JonBunator would any public API change in either senario?

Thanks for your response. Option one would require changes in MudButton: making buttons square when they have no text. Basically a rectangular MudIconButton.
It would look like this:
grafik
and not like this:
grafik

Option two would not require changes in MudButton or MudIconButton. I just would have to add mor styles to _buttongroup.scss. This would result in an increase in css LOC though.

@mikes-gh
Copy link
Contributor

Hmm. @henon do have an opinion on this. My first thoughts are changing button styles might have an impact we will not immediately be aware of. However the button shrinking to content seems like desired and predictable behaviour to me so I might be inclined to try that route and have a good look around the examples to see if it broke any layout.

@henon
Copy link
Collaborator

henon commented May 14, 2021

Sure mike, I agree.

@JonBunator
Copy link
Member Author

JonBunator commented May 14, 2021

Alright, I will create a seperate PR for this.
Edit: As discussed in #1557 this is not wanted anymore.

@JonBunator JonBunator marked this pull request as ready for review May 20, 2021 16:45
@henon
Copy link
Collaborator

henon commented May 21, 2021

Looking great so far.

Usually button group has an optional selection feature too. No selection, exclusive selection, multi-selection. At least I know it from Vuetify like that: https://vuetifyjs.com/en/components/button-groups/. The button group has a two-way bindable hashset of selected indices.

Do you want to add that feature + tests also?

@JonBunator
Copy link
Member Author

Thanks for your feedback!
I think the button group in vuetify only allows toggle buttons to be used. In contrast this component also allows MudButton and MudIconButton. I don't think it would make sense to add these parameters because it woud be confusing if you don't use toggle buttons in the button group.
I would rather create a seperate component called MudToggleIconButtonGroup for this. For example material-ui also uses two different components for this: ButtonGroup and ToggleButtonGroup
I wouldn't create a seperate doc page for this. I would expand the existing MudToggleIconButton page and add examples explaining the MudToggleIconButtonGroup. Exactly how it was done with the MudRadioGroup. Because MudToggleIconButton and MudToggleIconButtonGroup need to work tightly together, this would be a better solution in my opinion.
This component would require changes in MudToggleIconButton in order to function.
What do you think about my suggestion?

@henon
Copy link
Collaborator

henon commented May 22, 2021

Sounds good, I totally agree.

@JonBunator
Copy link
Member Author

Alright, I can start a PR after this PR is merged. That way I can reuse the css classes.

@henon henon merged commit 56d431e into MudBlazor:dev May 22, 2021
@JonBunator JonBunator deleted the feature/button-group branch May 22, 2021 12:44
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.

Implement a ButtonGroup control
4 participants