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

feat(select): implement select all #746

Merged
merged 4 commits into from
Dec 15, 2023
Merged

feat(select): implement select all #746

merged 4 commits into from
Dec 15, 2023

Conversation

ogunb
Copy link
Contributor

@ogunb ogunb commented Nov 30, 2023

Closes #520

@leventozen
Copy link
Member

leventozen commented Nov 30, 2023

Hey @ogunb, have you had the opportunity to check the performance and usability with a large number of options? I previously commented on this issue in our discussion about the select component problem we experienced more recently. You can find my comment in another PR here.

@ogunb
Copy link
Contributor Author

ogunb commented Dec 6, 2023

Sorry for the late reply, haven't had much time. Here 1000 items:
Screenshot 2023-12-06 at 12 38 22

I don't think we need any big refactors in this task.

Feel free to suggest improvements though.


Also I realized clear all button removes disabled options. Is this intended? Should it stay that way? @buseselvi

@buseselvi
Copy link
Contributor

@ogunb If there is a selected disabled option, clear all shouldn't clear them. Can we fix this here? Thank you for noticing. 🙏🏻

@leventozen
Copy link
Member

leventozen commented Dec 7, 2023

Sorry for the late reply, haven't had much time. Here 1000 items: Screenshot 2023-12-06 at 12 38 22

I don't think we need any big refactors in this task.

Feel free to suggest improvements though.

Also I realized clear all button removes disabled options. Is this intended? Should it stay that way? @buseselvi

Looks good imo but let's add one more check. Since we don't have any demo projects on hand right now, we can ask for @AykutSarac to test it in his existing project before merging.

@AykutSarac
Copy link
Member

Looks good imo but let's add one more check. Since we don't have any demo projects on hand right now, we can ask for @AykutSarac to test it in his existing project before merging.

Works fine at our project.

@buseselvi
Copy link
Contributor

Good job! 🚀 I have a few notes:

  • Can we make the "Select All" title the same font size as other titles in Storybook?
  • And I think it would be good to add another example to Storybook without the disabled option.
  • Also, yesterday at the meeting we noticed a small bug, when we clicked twice to select all, the checkbox changes like in the image below, can we fix it? 🙏🏻 @ogunb
image

@leventozen leventozen merged commit caf155e into next Dec 15, 2023
7 checks passed
@leventozen leventozen deleted the 520-select-all branch December 15, 2023 10:36
Copy link

🎉 This PR is included in version 2.4.0-beta.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

This was referenced Jan 25, 2024
Copy link

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'Select All' for Select Component
5 participants