[Portal] Migrate to React hooks#15399
Conversation
|
Hi @eps1lon const setMountNode = () => {
if (disablePortal) {
+ // mountNodeRef.current = ReactDOM.findDOMNode(ref).parentElement;
return;
}
mountNodeRef.current = getContainer(container, getOwnerDocument(ref).body);
};Can you kindly point me in the right direction? |
|
@material-ui/core: parsed: -0.23% 😍, gzip: -0.10% 😍 Details of bundle changes.Comparing: 7869aac...8560282
|
oliviertassinari
left a comment
oliviertassinari
left a comment
There was a problem hiding this comment.
I have tried to complete the effort. It's ready for a second phase of reviews :)
|
Thanks a lot, @oliviertassinari for your help, really appreciate..!! |
|
@gautam-pahuja I wish I could have guided you in the effort. But I didn't know how to handle the problem either, I only had a vague idea of what we could try and what could work. After hammering the problem for two hours, I could find a working solution. I'm excited about the |
| const Portal = React.forwardRef(function Portal(props, ref) { | ||
| const { children, container, disablePortal, onRendered } = props; | ||
| const [mountNode, setMountNode] = React.useState(null); | ||
| const childRef = React.useRef(null); |
There was a problem hiding this comment.
Just curious, do these explicitly need to be null?
There was a problem hiding this comment.
Reduces the number of overloads. React will set it to null when cleaning up so you end up with T | undefined | null if you don't initialize it. But I don't see a problem if you just compare weakly against nullish i.e. ref.current != null.
There was a problem hiding this comment.
Sebastian is right on spot. I have noticed the null / undefined difference in the tests that assert the ref values. It's inconsistent with the codebase. I'm reverting back to const childRef = React.useRef();. Thanks.
There was a problem hiding this comment.
Hum, actually, I see a couple of different usage in the codebase:
React.useRef(undefined);x 1React.useRef(null)x 10React.useRef()x38
There was a problem hiding this comment.
Yeah, that’s why I asked :P As long as we stick to one I’m fine with either blank or null
There was a problem hiding this comment.
If I understand Sebastian point, it's better in the demos to use null to reduce the number of types to write.
There was a problem hiding this comment.
I'm going with: null for the demos (improve TypeScript DX), nothing for the core (improve bundle size) and weak checks !=. It seems to be the already used standard, I only have to fix a few exceptions.
There was a problem hiding this comment.
For refs to components I would prefer useRef(null). This is equivalent to the old createRef style prevalent in classes:
const createdRef = React.createRef(); // createdRef.current === null
const usedRef = React.useRef(null); // usedRef.current === null
const usedUndefinedRef = React.useRef(); // usedUndefinedRef.current === undefinedThere was a problem hiding this comment.
OK, good for me. The bundle size increase is negligeable. I will update the pull request to match createRef. It's simpler.
|
Wow...Thanks a lot, @oliviertassinari ...This looks like way different to what I originally tried to do, I guess I have to work on my react-hooks skills before picking anything else up 😅 . |
oliviertassinari
added
the
breaking change
@gautam-pahuja Portal was a deceptively hard component to convert to Hooks. |
oliviertassinari
added
scope: portal
7 participants
This PR is a part of #15231
Breaking change
Only accept one element child when
disablePortalis set.