[TreeView] Make the cancellable event types public #14992
Conversation
flaviendelangle
added
typescript
component: tree view
| @@ -3,3 +3,7 @@ export type MuiCancellableEvent = { | |||
| }; | |||
|
|
|||
| export type MuiCancellableEventHandler<Event> = (event: Event & MuiCancellableEvent) => void; | |||
|
|
|||
| export const shouldSkipEventHandler = (event: MuiCancellableEvent): boolean => { | |||
There was a problem hiding this comment.
I'd be interested to have your opinion on the name of this method.
My logic behind having it instead of accessing .defaultMuiPrevented directly is that we will probably have a .defaultBasePrevented at some point and having a util that check both will be nice.
There was a problem hiding this comment.
I'm unsure about this assumption. 🤔
I think that on MUI X packages we could have defaultMuiPrevented and on Base UI X it could be defaultBasePrevented, but when consuming this library internally, we could add the defaultMuiPrevented prop in those cases.
This would allow us to keep the boolean event flag without creating a need for this function. 🤔
I'm always vary of extra functions increasing the call stack. 🙈 😆
WDYT? 🤔
There was a problem hiding this comment.
But the MUI X version will be built on top of the Base UI X version.
And all the Base UI X code will check for .defaultMuiPrevented, not for .defaultMuiPrevented.
So someone using the MUI X version will likely not have a correct behavior if he uses .defaultMuiPrevented, unless:
- We re-implement all the prevention in the MUI X wrapper (not scalable at all IMHO)
- The Base UI X also checks for
.defaultMuiPrevented(super ugly 😬 ) - The Base UI X package (and Base UI) have a way to customize the name of the property to check when preventing a custom behavior (so that the MUI layer can change magically change all the
.defaultBasePreventedto be.defaultMuiPrevented
I guess the core team will have the same topic, not sure if they discussed it...
One could say that we should just all migrate to .defaultBasePrevented or even rename it to something more generic.
There was a problem hiding this comment.
If we implement a solution that means any given package always have to check only on attribute on the event, then I agree that the method is not needed.
I'll remove it from this PR since it's value is hypothetical for now.
There was a problem hiding this comment.
Maybe we could ask @mui/material-ui if they have thought about this? 🤔
I guess the core team will have the same topic, not sure if they discussed it...
One could say that we should just all migrate to .defaultBasePrevented or even rename it to something more generic.
This is a great idea. 👍
It would be nice to know the Base and Material plan in this regard. 🤞
cc @mui/base-ui I see that there are usages of defaultMuiPrevented in your codebase.
Is there any plan to change it? Or is it here to stay?
There was a problem hiding this comment.
@michaldudak Oh, ok, this is new. The history of .defaultMuiPrevented:
- 2019: https://github.com/mui/material-ui/pull/17941/files#diff-804dd912f2ccdef39076fb6213816137debd3d39c0f32e940792bdaec8efbb8cR335. One event that bubble up between two different swipeable drawer. The first one that catches the event excludes the others with:
event.muiHandled = true;(as the event bubble, the value is retained). - 2020:
event.defaultMuiPrevented[Autocomplete] Add ability to override/compose key down events in autocomplete material-ui#19887. We had a different problem to solve, about allowing developers to opt-out of native event logic. We copied downshift [Autocomplete] Add ability to override/compose key down events in autocomplete material-ui#19887 (comment) which also happened to be the same as 1. - 2021: The data grid was using
event.stopPropagation()for the same use case as 2. it was a mess, we copied the solution from 2. [RFC] Use event.defaultMuiPrevented to prevent default events #1403 - 2024:
event.preventBaseUIHandler();
I think we should align 2. 3. and 4. toward the same pattern. As for use case 1. it's a private API, so it's something else.
If we go with 4. then I guess it would looks something like event.preventXGridHandler();. Meaning
- a function call, not a flag (which can be nice to validate that this is indeed supported, good to catch regression, but also maybe more boilerplate/bundle size to setup).
- not a function named using the org (today called mui) but one per project: BaseUI XGrid, XTreeView, XCharts. I doubt we will see a Mui prefix as the logic will be either in Base UI or X.
There was a problem hiding this comment.
If we name the method preventXGridHandler (or preventXHandler but one per project can be nice for very specific use case like when you have a picker inside a grid), then we don't have any naming issue between MUI X and Base UI X 👌
There was a problem hiding this comment.
With a 2nd thought on it, I fear that it will be hard for X to migrate to a method like preventXGridHandler because of the lesser unified structure of our codebase.
I'd be in favor of not introducing any new complexity and just rename the property .defaultXPrevented or .defaultXGridPrevented if we want a per product property.
If we go for .defaultXPrevented we keep the interfaces in the shared package but remove my method which will be useless.
If we go for .defaultXGridPrevented then we don't put anything in the shared package
There was a problem hiding this comment.
@alexfauquette would prefer to go with .defaultXGridPrevented to take a step forward in the direction of independant X products.
There was a problem hiding this comment.
Updated
- no more logic in
@mui/x-internals - exported variable are renamed to prepare for the renaming of the property
Follow up:
- rename
defaultMuiPreventedintodefaultXTreeViewPrevented(can be done during alpha)
flaviendelangle
force-pushed
the
cancellable-event
branch
2 times, most recently
from
f6fde17 to
6ee4600
Compare
|
Deploy preview: https://deploy-preview-14992--material-ui-x.netlify.app/ |
flaviendelangle
force-pushed
the
cancellable-event
branch
from
6ee4600 to
2fc3a06
Compare
flaviendelangle
force-pushed
the
cancellable-event
branch
from
2fc3a06 to
1652b00
Compare
flaviendelangle
force-pushed
the
cancellable-event
branch
from
1652b00 to
b231dde
Compare
Part of #14985
While writting the example for the issue, I noticed that I needed
MuiCancellableEventHandlerto have a correct typing but that this type was internal.Given that we will have the same topic on other X packages (pickers need to work to better support this pattern), I moved the logic to the shared package before re-exporting it.