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

fix(segmented button): Scale Segmented Button doesn't respect controlled selected state (#2166) #2384

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

glebbo-dev
Copy link
Contributor

@glebbo-dev glebbo-dev commented Feb 5, 2025

Fixes #2166
Reason for this issue was that segmented button has never known when selected segment has been changed. Therefore I had to send an event when segment was selected outside the scale component and react on this event in segment button.

Copy link

netlify bot commented Feb 5, 2025

Deploy Preview for marvelous-moxie-a6e2fe failed.

Name Link
🔨 Latest commit 70f030a
🔍 Latest deploy log https://app.netlify.com/sites/marvelous-moxie-a6e2fe/deploys/67a33a34c662a7000881f161

@@ -99,6 +99,15 @@ export class SegmentedButton {
this.setState(tempState);
}

@Listen('scaleSelectionChanged')
selectionChangedHandler() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will get fired to many times, since the segment being unselected and selected will fire this. so it might lead to other inconsistencies. As this would create an additional place for the segments state manipulation
the current setup communicates the selection of a segment using the 'scale-change' event to the parent semnet-button I think it is better to use the same event for both the programmatic and click change of the selection state.

Comment on lines +86 to +90
@Watch('selected')
selectionChanged() {
emitEvent(this, 'scaleSelectionChanged');
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO the watch approach seems promising, and it would be much better to have the scaleClick event emit here on change of the 'selected' state. This makes the click and programmatic change of the selection land in the same place.

@amir-ba
Copy link
Collaborator

amir-ba commented Feb 5, 2025

also run the lint and formatter npm command before you push your changes , otherwise the build pipeline will fail. Currently the preview deployment pipeline has some issues , but the build pipeline should work if you fix the linter and prettier issues.

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.

Scale Segmented Button doesn't respect controlled selected state
2 participants