-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[DataGrid] Refactor: create base Checkbox #16445
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
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
79d1961
refactor: create base Checkbox
romgrk f988128
lint
romgrk e726b9d
Merge branch 'master' into refactor-agnostic-checkbox
romgrk 6530e60
lint
romgrk ba08681
Merge branch 'master' into refactor-agnostic-checkbox
romgrk 89ec973
lint
romgrk b0382f6
Merge branch 'master' into refactor-agnostic-checkbox
romgrk 7dbadb8
fix: autoFocus & density
romgrk 2ed73be
Merge branch 'master' into refactor-agnostic-checkbox
romgrk 39a95f9
lint
romgrk 5a7d4ed
fix: style
romgrk 7c41e72
lint
romgrk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@romgrk Hope it's okay to ask it directly here:
If I'm not mistaken I can't override the
baseCheckbox
slot in a good way anymore (without diving into the source code here).I have to specifically recreate this conditional logic so that it works in the "Manage columns" overlay.
Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wdym by override? What's the use-case?
Base slots are indeed expected to have a bit of friction to overwrite, these are meant for design-system authors to re-implement so they're more low-level than what the average end-user would use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use-case is, that we have custom styling for most of the controls, which we also want in our DataGrid as good as possible.
When reading the docs I would expect this to work:
In v7 it was more or less a direct mapping from slot to MUI component but in v8 there's a whole new layer of complexity because every base slot may or may not be composed from multiple components now.
From my perspective I would expect something like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry there's not way around the added complexity. We want the datagrid to be usable with other design-systems, which means material-ui needs to go through a standardized intermediate layer, the base typings. If all you care about is styling, you can still pass material props via the
material={{ ... }}
prop, possibly viaslotProps
. Otherwise, forking/material/index.tsx
is best, it should be considered like the reference to implement custom bindings, either to material or to shadcn/mantine/etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it works for you, you could also wrap over the base checkbox instead of wrapping over the material-ui checkbox.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get your reasoning behind it but for me it gives the impression that other design-systems are "overvalued" compared to the native one.
From a pure consumer-perspective it should be easy to adjust the native components and hard to integrate another design-system.
Rough thought: Is it worth to make it harder for 99% of the people when 1% actually use another design-system.
This is highly conceptual, so I guess we can leave it here. 😅
Thanks for the additional technical input, how to solve it. 👍