Skip to content

Conversation

@Han5991
Copy link
Contributor

@Han5991 Han5991 commented May 22, 2025

Description

Changes

  • Added use-selection custom hook implementation in [@mantine/hooks].
  • Fixed the isSomeSelected logic to handle empty data cases correctly and added related test cases.
  • Introduced comprehensive tests for use-selection in [@mantine/hooks].
  • Improved documentation in [@mantine/docs], including:
    • Storybook setup for showcasing components and hooks.
    • Initial version of demos for better usage reference.
  • Updated [@mantine/core] to use @mantine/hooks for state management.
  • Updated [@mantine/dates] to use @mantine/hooks for state management.

@Han5991 Han5991 requested a review from rtivital May 22, 2025 22:07
@jvdsande
Copy link
Contributor

For having implemented such a hook in my own project, I'd suggest the following should be added:

  • if data changes, remove from selected any entry that is not in data anymore

@rtivital is that going too far for what you had in mind?

@rtivital
Copy link
Member

Yeah, makes sense, I'll check how this should be handled, probably as an option

@jvdsande
Copy link
Contributor

At the very least I think isSomeSelected should check that at least one selected entry is present in data

@Han5991
Copy link
Contributor Author

Han5991 commented May 24, 2025

@jvdsande
Thanks to you, we've added related notes code and test code.

- Updated `isSomeSelected` to check if selected items exist in the current data.
- Added tests to verify `isSomeSelected` returns correct values for various selection states.
@Han5991 Han5991 force-pushed the feature/use-selection branch from e99205c to 1c4dd25 Compare May 24, 2025 16:40
@Han5991
Copy link
Contributor Author

Han5991 commented May 26, 2025

Hi @rtivital

I noticed that PR #7873 had its target branch changed to the next version branch, while my PR #7872 (which implements similar useSelection functionality) is still targeting the current branch.

I'm curious about the decision process - was there something specific about the implementation approach or timing that influenced which PR gets included in which version? I'd love to understand the criteria so I can better align my contributions with the project roadmap.

Both PRs seem to tackle similar functionality, so I'm wondering if there are plans to:

  • Include both implementations for different use cases?
  • Choose one approach over the other?
  • Merge the best aspects of both?

I'm happy to collaborate or adjust my PR based on the project's needs. Thanks for all the great work on this project!

Best regards

@rtivital
Copy link
Member

On Discord you've mentioned that you want to implement it out of interest. The other person requested to develop this feature before you and submitted PR. Implementations are not very different, so, from my side, it seems to be more polite to accept the PR that was assigned to someone initially. Nothing wrong with the code.

@Han5991
Copy link
Contributor Author

Han5991 commented May 26, 2025

On Discord you've mentioned that you want to implement it out of interest. The other person requested to develop this feature before you and submitted PR. Implementations are not very different, so, from my side, it seems to be more polite to accept the PR that was assigned to someone initially. Nothing wrong with the code.

@rtivital
Thank you for the explanation. I understand your reasoning and I fully respect your decision as the maintainer. I’ll go ahead and close this PR. Appreciate your time and feedback!

@Han5991 Han5991 closed this May 26, 2025
@Han5991 Han5991 reopened this May 26, 2025
@Han5991 Han5991 closed this May 26, 2025
@Han5991 Han5991 deleted the feature/use-selection branch June 22, 2025 10:00
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.

3 participants