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

SelectButton: inverse unselectable behaviour #3708

Closed
tugcekucukoglu opened this issue Mar 9, 2023 · 11 comments
Closed

SelectButton: inverse unselectable behaviour #3708

tugcekucukoglu opened this issue Mar 9, 2023 · 11 comments
Assignees
Labels
Type: Breaking Change Issue contains a breaking change related to a specific component
Milestone

Comments

@tugcekucukoglu
Copy link
Member

No description provided.

@tugcekucukoglu tugcekucukoglu added Type: Bug Issue contains a bug related to a specific component. Something about the component is not working Type: Breaking Change Issue contains a breaking change related to a specific component labels Mar 9, 2023
@tugcekucukoglu tugcekucukoglu added this to the 3.24.1 milestone Mar 9, 2023
@tugcekucukoglu tugcekucukoglu self-assigned this Mar 9, 2023
@tugcekucukoglu
Copy link
Member Author

Fixed via #3610

@tugcekucukoglu tugcekucukoglu removed the Type: Bug Issue contains a bug related to a specific component. Something about the component is not working label Mar 9, 2023
@rubjo
Copy link
Contributor

rubjo commented Mar 24, 2023

@aguyfromdenmark @tugcekucukoglu

Current (3.26.0) behaviour for unselectable true seems to be that the SelectButton cannot be unselected. Does that make sense to you? :)

@jnoordsij
Copy link

I'm utterly confused by this change; isn't the old behavior exactly what you would expect, and the new behavior wrong? Moreover, even if you do think this is correct, reversing the behavior is a major breaking change of behavior on a minor version, which will probably end up in lots of people being confused.

I would suggest rolling back this changeset and introducing a more clearly named prop (e.g. allowEmpty or allowClear) instead, deprecating the old one and removing it in a future major update.

@aguyfromdenmark
Copy link
Contributor

aguyfromdenmark commented Apr 13, 2023

@rubjo Yeah, that makes perfect sense. That is the behaviour I introduced in my pull request #3610 , that @tugcekucukoglu merged on the 9th of March. Before that, @jnoordsij , you were able to completely unselect all options in the SelectButton when setting unselectable to true, which to me, is the oppsite behavior of what I expect when I set unselectable to true.

There is a chance that I am misunderstanding what this thread is about, but it am genuinely confused to as what the ongoing "problem" seems to be.

@jnoordsij
Copy link

jnoordsij commented Apr 13, 2023

I think the problem is indeed the mixed interpretation of what unselectable = true means; I would interpret it as "the button allows unselecting", which is exactly what it previously did. I think this behavior also matches the description from the docs: Whether selection can be cleared.. So setting it to true would allow clearing all options, setting it to false would disallow this.

I do agree the reverse interpretation migth be logical in some sense as well; hence my proposal to revert to the previous (documented) state and to use a more clear naming to go forward instead.

@rubjo
Copy link
Contributor

rubjo commented Apr 13, 2023

Yeah, support @jnoordsij’s suggestion here.

@aguyfromdenmark
Copy link
Contributor

@jnoordsij That makes sense. It can be read both ways. Now, it's a long time since I made my pull request, so I can't say for sure. But I think the docs might have said something else back then, because I was flabbergasted when I found out that it did the inverse of what I expected. I will always read unselectable = true as "this cannot be be cleared".

So I guess this is something for someone else to decide what to do about,

@rubjo
Copy link
Contributor

rubjo commented Apr 13, 2023

I read unselectable=true to «can be unselected». 🙂

@aguyfromdenmark
Copy link
Contributor

I read unselectable=true to «can be unselected». 🙂

And there ya' have it 🤣

@rubjo
Copy link
Contributor

rubjo commented Apr 13, 2023

Want to chime in @tugcekucukoglu ?

To unselect something is to clear something. Hence, unselectable === clearable.

@rwparris2
Copy link

+1 to @jnoordsij 's suggestion.

Definitely don't reverse the behavior as that would break all existing code using that property.

Deprecate it, replace the naming with something less ambiguous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking Change Issue contains a breaking change related to a specific component
Projects
None yet
Development

No branches or pull requests

5 participants