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

Button Group: Add new component #2110

Merged
merged 38 commits into from
Apr 19, 2024
Merged

Button Group: Add new component #2110

merged 38 commits into from
Apr 19, 2024

Conversation

ElishaSamPeterPrabhu
Copy link
Collaborator

@ElishaSamPeterPrabhu ElishaSamPeterPrabhu commented Feb 2, 2024

Description

  • Used existing modus buttons to be attached as a group when given under the tag modus-button-group.
  • Updated UI of button with the property , toggelable for toggling the selected feature and a state buttonPosition to handle the UI of the button based on postion (left,center and right)
  • Has two style the default one which doesn't have a selected state and the other style single which allows button to have a selected state

References: #1558

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

https://deploy-preview-2110--moduswebcomponents.netlify.app/?path=/docs/components-buttongroup--default-selection-button-group

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

Copy link

netlify bot commented Feb 2, 2024

Deploy Preview for moduswebcomponents ready!

Name Link
🔨 Latest commit 1cdda49
🔍 Latest deploy log https://app.netlify.com/sites/moduswebcomponents/deploys/66219b75cd5c5500081da536
😎 Deploy Preview https://deploy-preview-2110--moduswebcomponents.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 27 (🟢 up 1 from production)
Accessibility: 98 (no change from production)
Best Practices: 92 (no change from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

@ElishaSamPeterPrabhu ElishaSamPeterPrabhu changed the title Issue 1558/button group Add new button group Component Feb 5, 2024
Copy link

@jmwood330t jmwood330t left a comment

Choose a reason for hiding this comment

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

Good tests here - and thanks.
Jim

@cjwinsor
Copy link
Contributor

cjwinsor commented Mar 7, 2024

@ElishaSamPeterPrabhu I found this example of a button group i think we can mimic how they handle the rounded borders.
https://github.com/goatui/components/tree/main/src/components/button-group

Also, we need to talk to Ewa about the style. The guidelines currently only show outline and thats all she wants us to implement... but it makes things interesting as the button group could accept any buttons naturally as they are configured if we wanted to.. mixing and matching styles would be odd but doable. The alternative being that we can have a different child component... modus-button-group-button sounds stupid though.

For now, see if you can manage handling the border radius using css variables like my example.

As for as the toggleable option, still considering that part of it.... I'm wondering if we could leverage type and have a toggle type.. I'll ask in the meeting tomorrow to see if @coliff has any issues or concerns.

I'd almost want to just have it be

<modus-button-group>
  <modus-button type="button"></modus-button>
  <modus-button type="toggle"></modus-button>
  <modus-button type="button"></modus-button>
</modus-button-group>

but this leads to people being able to set random sizes, colors, and styles... which would be stupid to do but they could do it...

With your current method or remapping, it could solve that, but it also adds complexity where we could simply just have a slot and throw the buttons in there and let the user deal with each buttons events individually.

Just putting all my current thoughts down as I haven't updated this PR with any of them yet.

@ElishaSamPeterPrabhu
Copy link
Collaborator Author

@ElishaSamPeterPrabhu I found this example of a button group i think we can mimic how they handle the rounded borders. https://github.com/goatui/components/tree/main/src/components/button-group

Also, we need to talk to Ewa about the style. The guidelines currently only show outline and thats all she wants us to implement... but it makes things interesting as the button group could accept any buttons naturally as they are configured if we wanted to.. mixing and matching styles would be odd but doable. The alternative being that we can have a different child component... modus-button-group-button sounds stupid though.

For now, see if you can manage handling the border radius using css variables like my example.

As for as the toggleable option, still considering that part of it.... I'm wondering if we could leverage type and have a toggle type.. I'll ask in the meeting tomorrow to see if @coliff has any issues or concerns.

I'd almost want to just have it be

<modus-button-group>
  <modus-button type="button"></modus-button>
  <modus-button type="toggle"></modus-button>
  <modus-button type="button"></modus-button>
</modus-button-group>

but this leads to people being able to set random sizes, colors, and styles... which would be stupid to do but they could do it...

With your current method or remapping, it could solve that, but it also adds complexity where we could simply just have a slot and throw the buttons in there and let the user deal with each buttons events individually.

Just putting all my current thoughts down as I haven't updated this PR with any of them yet.

I tried the slotting method from the above github link am unable to modify the css properties of the modus button from the button group.

In my opinion user's mismatching the button style should be forbidden, as I consider the button group a separate component and shouldn't have different buttons present on a list of buttons inside the button group .Hence showing the outline alone is fine.

Regarding the toggleable functionality , since we are toggling between one button and another shouldn't all the buttons of a button group carry the same type either toggle or button?

@cjwinsor
Copy link
Contributor

@ElishaSamPeterPrabhu We will still need the selection-type prop so by default its just regular buttons without the isActive being used and the single mode where only one is active at a time.

@@ -0,0 +1,9 @@
export const OUTLINE_STYLE = 'outline';
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these aren't used anymore

@@ -0,0 +1,19 @@
import {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cleanup unused. Those we should keep Selection Type as we should have that property.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleaned up

buttons.forEach((button: HTMLModusButtonElement) => {
button.disabled = this.disabled;
button.buttonStyle = 'outline';
button.type = 'toggle';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should only be type toggle when selection type is single, and we would need to add an event listener to make sure only one button is clicked at a time. So you can keep track of selectedButtons and set isActive to false on any in that array (multiple select coming later would probably need this to be an array).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

isActive is a state in modus-button, so are you saying to modify it from button groups?

@ElishaSamPeterPrabhu
Copy link
Collaborator Author

I removed the selection-type previously because I thought we didn't finalize on the selection methods .
For now am going with two types single and the default one.

@cjwinsor cjwinsor changed the title Add new button group Component Button Group: Add new component Apr 12, 2024
ElishaSamPeterPrabhu and others added 10 commits April 15, 2024 10:46
…oss/modus-web-components into issue-1558/Button-Group

# Conflicts:
#	stencil-workspace/src/components/modus-icons/readme.md
#	stencil-workspace/src/components/modus-text-input/readme.md
…nents into issue-1558/Button-Group

# Conflicts:
#	angular-workspace/ng14/projects/trimble-oss/modus-angular-components/src/lib/stencil-generated/components.ts
#	angular-workspace/ng14/projects/trimble-oss/modus-angular-components/src/lib/stencil-generated/index.ts
#	angular-workspace/ng15/projects/trimble-oss/modus-angular-components/src/lib/stencil-generated/components.ts
#	angular-workspace/ng15/projects/trimble-oss/modus-angular-components/src/lib/stencil-generated/index.ts
#	angular-workspace/ng16/projects/trimble-oss/modus-angular-components/src/lib/stencil-generated/components.ts
#	angular-workspace/ng16/projects/trimble-oss/modus-angular-components/src/lib/stencil-generated/index.ts
#	angular-workspace/ng17/projects/trimble-oss/modus-angular-components/src/lib/stencil-generated/components.ts
#	angular-workspace/ng17/projects/trimble-oss/modus-angular-components/src/lib/stencil-generated/index.ts
#	react-workspace/react-17/src/components/stencil-generated/index.ts
#	react-workspace/react-18/src/components/stencil-generated/index.ts
@cjwinsor cjwinsor merged commit 2ce34dd into main Apr 19, 2024
9 checks passed
@cjwinsor cjwinsor deleted the issue-1558/Button-Group branch April 19, 2024 01:04
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.

None yet

3 participants