[ClickAwayListener] Fix support for portal #20406
Conversation
NMinhNguyen
force-pushed
the
clickaway
branch
2 times, most recently
from
36a8b22
to
9fa8c65
Compare
Details of bundle changes.Comparing: 27ada70...e7bcb58 Details of page changes
|
packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
Outdated
Show resolved
Hide resolved
oliviertassinari
force-pushed
the
clickaway
branch
from
aa6a504
to
010995c
Compare
packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
Outdated
Show resolved
Hide resolved
packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
Outdated
Show resolved
Hide resolved
|
Another opportunity: // Inline this into the effect and remove useCallback
const handleTouchMove = React.useCallback(() => {
movedRef.current = true;
}, []); |
packages/material-ui/src/ClickAwayListener/ClickAwayListener.js
Outdated
Show resolved
Hide resolved
Sure, happy to go for it too. Let's get this figure down 😈 ClickAwayListener: {
parsed: 3839,
gzip: 1540
},https://material-ui.com/size-snapshot. I wish we can go further below: https://bundlephobia.com/result?p=react-onclickoutside. |
|
DM'ed you on Twitter but you prob haven't checked. I can implement these changes myself but I've stopped touching the branch to avoid conflicts for either of us. |
|
Basically, I want to go after https://github.com/Hacker0x01/react-datepicker/blob/efffb475290b41793f9bccbf09a8a0304975951f/package.json#L116, selling 1. smaller bundle size 2. safe long term bet, 3. written in hooks, 4. better documented, 5. fewer bugs. |
|
It might be slightly trickier to move to hooks due to us using
const clickAwayProps = useClickAway({ onClick })
<div {...clickAwayProps}/>
|
It sounds like a great option but what problem would a hook API solve? |
point 3 in your comment above 😆 |
Well, the core is written in hooks, this was a positive argument. Does it need to leak to the API level? |
My bad, I misread. |
|
In #18689 (comment), the author mentions easier composition as an argument for a hook API. I'm not sure I buy into it. |
oliviertassinari
added
bug 🐛
oliviertassinari
changed the title
docs/src/pages/components/click-away-listener/click-away-listener.md
Outdated
Show resolved
Hide resolved
oliviertassinari
added
PR: ready to ship
and removed
PR: out-of-date
|
@NMinhNguyen Great work! |
| // True-negative, we don't have enough information to do otherwise. | ||
| expect(handleClickAway.callCount).to.equal(1); |
There was a problem hiding this comment.
@NMinhNguyen This is undesired behavior, no? Since the target handler stops the propagation we shouldn't register a click to begin with?
There was a problem hiding this comment.
I think this particular test was written by @oliviertassinari - 1a306e2 who may be able to provide more clarity on this. Sorry, it's been a while, I don't quite remember the details now. But could be to do with event.nativeEvent.stopImmediatePropagation(); and event.stopPropagation();
There was a problem hiding this comment.
Ah check this actually #20406 (comment)
There was a problem hiding this comment.
I'm not asking why this is happening bug if this is desired or undesired behavior.
Seems to me that having the React event system work closer to how the DOM event system work is desired.
There was a problem hiding this comment.
Right. I'm personally not sure, hence why I raised that question in the first place, and I let @oliviertassinari make a call. I'll probably have to defer to Olivier on your question though.
There was a problem hiding this comment.
Had some further thoughts actually. Given it says "True-negative" in the comment, I'm inclined to say it's undesired.
There was a problem hiding this comment.
@eps1lon To further clarify what I had in the back of my mind during the implementation:
- I shouldn't have used: "True-negative", it wasn't the right description, backward actually, sorry for the confusion. It should have been "False-positive", "False" as the behavior is wrong, and "positive" as it raises when it shouldn't.
- The outcome was a hard limitation of the solution we took. We needed React to have a different behavior to have a True-negative (the desired outcome).
Having a closer look at #22105, it seems that the new behavior of React is to do event.nativeEvent.stopImmediatePropagation(); when event.stopPropagation(); is called on the synthetic event, which is perfect for us, we no longer have a "False-positive" :D.
There was a problem hiding this comment.
Yeah, personally I try to avoid "true-negative" or "false-positive" since they are meaningless unless you specific to what pool "true" and "negative" belong to.
I think it makes more sense to use the following mapping:
- behavior in a failing test that shouldn't fail: unexpected and undesired behavior
- behavior in a failing test that should fail: unexpected and desired
- behavior in a passing test that shouldn't pass: expected and undesired behavior
- behavior in a passing test that should pass: expected and desired behavior
(un)expected behavior is basically what you write in your test and (un)desired behavior is what your (un)expected behavior should eventually become.
So here we had expected but undesired behavior which is unexpected but desired behavior in react@next and will become expected and desired behavior in #22105 (comment)
There was a problem hiding this comment.
For posterity, https://reactjs.org/blog/2020/08/10/react-v17-rc.html#fixing-potential-issues explains how event.stopPropagation() works in React 17.
Closes #18586
I could've also added the below test for completeness, but not sure if there's any value: