Skip to content

feat(ui): add ToggleButton and SortButton components#1165

Merged
guoda-puidokaite merged 9 commits intomainfrom
guoda-toggle-button
Sep 22, 2025
Merged

feat(ui): add ToggleButton and SortButton components#1165
guoda-puidokaite merged 9 commits intomainfrom
guoda-toggle-button

Conversation

@guoda-puidokaite
Copy link
Copy Markdown
Contributor

@guoda-puidokaite guoda-puidokaite commented Sep 18, 2025

Summary

Changes Made

  • Fulfilled all ACs.

Checklist

  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • 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.
  • I have made corresponding changes to the documentation (if applicable).
  • My changes generate no new warnings or errors.
  • I have created a changeset for my changes.

Testing

Reviewers: Please see Quality Assurance section in the main ticket. 🙏

PR Manifesto

Review the PR Manifesto for best practises.

@guoda-puidokaite guoda-puidokaite self-assigned this Sep 18, 2025
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Sep 18, 2025

🦋 Changeset detected

Latest commit: f04d81d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@cloudoperators/juno-ui-components Minor
@cloudoperators/juno-app-carbon Patch
@cloudoperators/juno-app-doop Patch
@cloudoperators/juno-app-example Patch
@cloudoperators/juno-app-greenhouse Patch
@cloudoperators/juno-app-heureka Patch
@cloudoperators/juno-app-supernova Patch
@cloudoperators/juno-app-template Patch
@cloudoperators/juno-messages-provider Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Sep 18, 2025

PR Preview Action v1.6.2
Preview removed because the pull request was closed.
2025-09-22 09:11 UTC

@guoda-puidokaite guoda-puidokaite changed the title feat(ui): initial toggle button feat(ui): add ToggleButton and SortButton components Sep 22, 2025
@guoda-puidokaite guoda-puidokaite marked this pull request as ready for review September 22, 2025 06:38
@guoda-puidokaite guoda-puidokaite requested review from a team and franzheidl as code owners September 22, 2025 06:38
@guoda-puidokaite
Copy link
Copy Markdown
Contributor Author

The ticket asks for all props to be extended from Button... It spams Storybook from the main component props and also, some props don't make sense when added e.g. icon. Or maybe it does? Idk. 😆

Screenshot 2025-09-22 at 08 40 53

TilmanHaupt
TilmanHaupt previously approved these changes Sep 22, 2025
Copy link
Copy Markdown
Contributor

@TilmanHaupt TilmanHaupt left a comment

Choose a reason for hiding this comment

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

That looks clean and perfect! Thank you.

To your question with the Icon - I guess you are right but its not really hurting for me. So its an option in extreme edge cases maybe.

Copy link
Copy Markdown
Collaborator

@andypf andypf left a comment

Choose a reason for hiding this comment

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

Looks good to me. However, I found two points that could potentially be improved.

Comment thread packages/ui-components/src/components/ToggleButton/ToggleButton.component.tsx Outdated
Comment thread packages/ui-components/src/components/ToggleButton/ToggleButton.component.tsx Outdated
Copy link
Copy Markdown
Collaborator

@andypf andypf left a comment

Choose a reason for hiding this comment

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

Great!

@guoda-puidokaite guoda-puidokaite merged commit afb808f into main Sep 22, 2025
16 checks passed
@guoda-puidokaite guoda-puidokaite deleted the guoda-toggle-button branch September 22, 2025 09:10
@taymoor89
Copy link
Copy Markdown
Contributor

taymoor89 commented Sep 22, 2025

I'm curious why we have to make ToggleButton a mixed of controlled and uncontrolled component rather than purely uncontrolled one, where we could've just taken list of options and the current value and we just select the correct option without maintaining an internal state and this could've also worked without useEffect.

Update:
ah I meant purely controlled instead of uncontrolled.

@taymoor89
Copy link
Copy Markdown
Contributor

One more case to consider, if the list of options change and the selected value does not match any option it would render a button without any label, shouldn't we reset selected value back to the first option as we do when initializing internal state of the component? Not sure what would be the good UX in this case.

@edda
Copy link
Copy Markdown
Contributor

edda commented Sep 22, 2025

I'm curious why we have to make ToggleButton a mixed of controlled and uncontrolled component rather than purely uncontrolled one, where we could've just taken list of options and the current value and we just select the correct option without maintaining an internal state and this could've also worked without useEffect.

The reason for making the components controlled is that in actual usage developers often want to use it in a controlled fashion (i.e. being able to set the value initially and also update the value later). We used to have more components that were uncontrolled, or could be either controlled or uncontrolled. But in practice it often produced problems. If you have an actual uncontrolled component and try to change the value after the initial mount you'll get an error message from React because this is not allowed. So our solution was to make all components controlled (for React) but some of them can behave like an uncontrolled component if the developer doesn't want to update state manually.

@andypf
Copy link
Copy Markdown
Collaborator

andypf commented Sep 22, 2025

I'm curious why we have to make ToggleButton a mixed of controlled and uncontrolled component rather than purely uncontrolled one, where we could've just taken list of options and the current value and we just select the correct option without maintaining an internal state and this could've also worked without useEffect.

The reason for making the components controlled is that in actual usage developers often want to use it in a controlled fashion (i.e. being able to set the value initially and also update the value later). We used to have more components that were uncontrolled, or could be either controlled or uncontrolled. But in practice it often produced problems. If you have an actual uncontrolled component and try to change the value after the initial mount you'll get an error message from React because this is not allowed. So our solution was to make all components controlled (for React) but some of them can behave like an uncontrolled component if the developer doesn't want to update state manually.

Hey, I was thinking - doesn't React already have a defaultValue prop? So, you can set the initial value and leave it uncontrolled afterward, right? Just wondering if that could be a simpler way to handle this.

@taymoor89
Copy link
Copy Markdown
Contributor

I'm curious why we have to make ToggleButton a mixed of controlled and uncontrolled component rather than purely uncontrolled one, where we could've just taken list of options and the current value and we just select the correct option without maintaining an internal state and this could've also worked without useEffect.

The reason for making the components controlled is that in actual usage developers often want to use it in a controlled fashion (i.e. being able to set the value initially and also update the value later). We used to have more components that were uncontrolled, or could be either controlled or uncontrolled. But in practice it often produced problems. If you have an actual uncontrolled component and try to change the value after the initial mount you'll get an error message from React because this is not allowed. So our solution was to make all components controlled (for React) but some of them can behave like an uncontrolled component if the developer doesn't want to update state manually.

I see your point but IMO the overhead to maintain a hybrid component is huge. Specially for the use case and the user experience of this specific ToggleButton component where IMO a developer will always want to be informed about the state change and will keep it in app/component state to be able to reflect these changes in the UI. If that holds true then making it a controlled component where the parent must supply value and list of options would simplify this component and we don't need to have an internal state and to think about syncing it when for example value and options props change.

@edda
Copy link
Copy Markdown
Contributor

edda commented Sep 22, 2025

I see your point but IMO the overhead to maintain a hybrid component is huge. Specially for the use case and the user experience of this specific ToggleButton component where IMO a developer will always want to be informed about the state change and will keep it in app/component state to be able to reflect these changes in the UI. If that holds true then making it a controlled component where the parent must supply value and list of options would simplify this component and we don't need to have an internal state and to think about syncing it when for example value and options props change.

Yes, you're right. We were thinking of DX here and wanted to offer developers the best of both worlds essentially. The way we do it currently allows people to use the components completely controlled or uncontrolled or in a weird hybrid way that normally isn't possible. But it does make the components more complex and maybe it's completely unnecessary, as you pointed out, since people will have their own app state anyway and then the internal component state is just duplicated information. We will discuss this and make a decision how we want to do this going forward.

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.

[Task] (ui): create SortButton Component [Task](ui): create ToggleButton component

5 participants