Skip to content

Conversation

urubatan
Copy link
Contributor

@urubatan urubatan commented Apr 6, 2023

When clicking on an inline option for a SelectMultiple that uses objects as options the behavior should be the same as when the options are strings.
This PR fixes the comparison for object values to be the same as for string values

What does this PR do?

changes one line in SelectMultipleValue to make sure the inline option clicked is deselected instead of being the only one selected for object options

Where should the reviewer start?

you can check the current error in storybook

https://storybook.grommet.io/?path=/story/input-selectmultiple-object-options--object-options

when clicking in any inline option that is the only one that is still selected, instead of deselecting only that option

What testing has been done on this PR?

I tested manually on my machine

all existing tests are still working

I didn't add more tests to confirm the bug because my javascript skills aren't good enough and I didn't know how to do that

How should this be manually tested?

opening the storybook for this PR you can verify the correct behavior

What are the relevant issues?

This PR fixes the issue #6737

Do the grommet docs need to be updated?

No, after this PR the docs are correct for all cases

Should this PR be mentioned in the release notes?

no need

Is this change backwards compatible or is it a breaking change?

it just fixes a bug

… clicked keeping the same behaviour as string options
@urubatan urubatan changed the title fir for SelectMultiple when using object values inline clicks fix for SelectMultiple when using object values inline clicks Apr 6, 2023
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! Would you be willing to add a unit test for this?

@urubatan
Copy link
Contributor Author

urubatan commented Apr 6, 2023 via email

@jcfilben
Copy link
Collaborator

jcfilben commented Apr 6, 2023

Sure, I'll check how to and will push the tests later today

Thanks! Feel free to reach out if you run into any issues

@urubatan
Copy link
Contributor Author

urubatan commented Apr 7, 2023

@jcfilben just pushed the test to validate the fix

@urubatan urubatan requested a review from jcfilben April 7, 2023 12:44
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

Nice work!

@jcfilben jcfilben requested a review from taysea April 7, 2023 15:32
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.

4 participants