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 the Filterchips #858

Merged
merged 4 commits into from
May 6, 2022
Merged

Fix the Filterchips #858

merged 4 commits into from
May 6, 2022

Conversation

RichardSto
Copy link
Contributor

Yeah thats a weird one. Apparently the Filterchips were broken when you press a bubble.
It doesn't really have a way to update chips from outside. This should be just a dumb component without state, BUT the "all" and "others" chip used in polyExplorer make this quite complicated logic which i wouldn't want to repeat.

Soooo I added another state to compare the current state to the original one, so when the defaultChip changes from outside it'll change on the inside. Yeah just a hack for now..

@RichardSto RichardSto requested review from JJ and g3o314 May 5, 2022 16:20
Copy link
Contributor

@JJ JJ left a comment

Choose a reason for hiding this comment

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

Would it be terribly out of scope to add a test that would fail in the previous situation, and not fail in this one? IIRC, coverage of tests for filterChips could be improved, also.

@RichardSto
Copy link
Contributor Author

Would it be terribly out of scope to add a test that would fail in the previous situation, and not fail in this one? IIRC, coverage of tests for filterChips could be improved, also.

Looking at this PR testing-library/react-testing-library#65 to see they actually implemented rerender. Love to see it :D

Copy link
Contributor

@g3o314 g3o314 left a comment

Choose a reason for hiding this comment

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

I'm not familiar with how this component works so I can't unfortunately provide feedback. Still, having the test for the fix it really neat and I guess that's good enough for me.

@RichardSto RichardSto merged commit b2de0c6 into main May 6, 2022
@RichardSto RichardSto deleted the fix-filterchips branch May 6, 2022 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants