-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat!: add StrictMode support #5687
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,12 +2,14 @@ import classNames from 'classnames'; | |
import PropTypes from 'prop-types'; | ||
import React, { useCallback } from 'react'; | ||
import Transition, { | ||
TransitionStatus, | ||
ENTERED, | ||
ENTERING, | ||
} from 'react-transition-group/Transition'; | ||
import transitionEndListener from './transitionEndListener'; | ||
import { TransitionCallbacks } from './helpers'; | ||
import triggerBrowserReflow from './triggerBrowserReflow'; | ||
import TransitionWrapper from './TransitionWrapper'; | ||
|
||
export interface FadeProps extends TransitionCallbacks { | ||
className?: string; | ||
|
@@ -92,19 +94,20 @@ const Fade = React.forwardRef<Transition<any>, FadeProps>( | |
const handleEnter = useCallback( | ||
(node) => { | ||
triggerBrowserReflow(node); | ||
if (props.onEnter) props.onEnter(node); | ||
props.onEnter?.(node); | ||
}, | ||
[props], | ||
); | ||
|
||
return ( | ||
<Transition | ||
<TransitionWrapper | ||
ref={ref} | ||
addEndListener={transitionEndListener} | ||
{...props} | ||
onEnter={handleEnter} | ||
childRef={(children as any).ref} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
> | ||
{(status, innerProps) => | ||
{(status: TransitionStatus, innerProps: Record<string, unknown>) => | ||
React.cloneElement(children, { | ||
...innerProps, | ||
className: classNames( | ||
|
@@ -115,7 +118,7 @@ const Fade = React.forwardRef<Transition<any>, FadeProps>( | |
), | ||
}) | ||
} | ||
</Transition> | ||
</TransitionWrapper> | ||
); | ||
}, | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import useTimeout from '@restart/hooks/useTimeout'; | |
import safeFindDOMNode from 'react-overlays/safeFindDOMNode'; | ||
import warning from 'warning'; | ||
import { useUncontrolledProp } from 'uncontrollable'; | ||
import useMergedRefs from '@restart/hooks/useMergedRefs'; | ||
import Overlay, { OverlayChildren, OverlayProps } from './Overlay'; | ||
|
||
export type OverlayTriggerType = 'hover' | 'click' | 'focus'; | ||
|
@@ -36,12 +37,6 @@ export interface OverlayTriggerProps | |
onHide?: never; | ||
} | ||
|
||
class RefHolder extends React.Component { | ||
render() { | ||
return this.props.children; | ||
} | ||
} | ||
|
||
function normalizeDelay(delay?: OverlayDelay) { | ||
return delay && typeof delay === 'object' | ||
? delay | ||
|
@@ -186,6 +181,10 @@ function OverlayTrigger({ | |
...props | ||
}: OverlayTriggerProps) { | ||
const triggerNodeRef = useRef(null); | ||
const mergedRef = useMergedRefs<unknown>( | ||
triggerNodeRef, | ||
(children as any).ref, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
); | ||
const timeout = useTimeout(); | ||
const hoverStateRef = useRef<string>(''); | ||
|
||
|
@@ -198,10 +197,9 @@ function OverlayTrigger({ | |
? React.Children.only(children).props | ||
: ({} as any); | ||
|
||
const getTarget = useCallback( | ||
() => safeFindDOMNode(triggerNodeRef.current), | ||
[], | ||
); | ||
const attachRef = (r: React.ComponentClass | Element | null | undefined) => { | ||
mergedRef(safeFindDOMNode(r)); | ||
}; | ||
|
||
const handleShow = useCallback(() => { | ||
timeout.clear(); | ||
|
@@ -270,7 +268,9 @@ function OverlayTrigger({ | |
); | ||
|
||
const triggers: string[] = trigger == null ? [] : [].concat(trigger as any); | ||
const triggerProps: any = {}; | ||
const triggerProps: any = { | ||
ref: attachRef, | ||
}; | ||
|
||
if (triggers.indexOf('click') !== -1) { | ||
triggerProps.onClick = handleClick; | ||
|
@@ -292,21 +292,17 @@ function OverlayTrigger({ | |
|
||
return ( | ||
<> | ||
{typeof children === 'function' ? ( | ||
children({ ...triggerProps, ref: triggerNodeRef }) | ||
) : ( | ||
<RefHolder ref={triggerNodeRef}> | ||
{cloneElement(children as any, triggerProps)} | ||
</RefHolder> | ||
)} | ||
{typeof children === 'function' | ||
? children(triggerProps) | ||
: cloneElement(children, triggerProps)} | ||
<Overlay | ||
{...props} | ||
show={show} | ||
onHide={handleHide} | ||
flip={flip} | ||
placement={placement} | ||
popperConfig={popperConfig} | ||
target={getTarget as any} | ||
target={triggerNodeRef.current} | ||
> | ||
{overlay} | ||
</Overlay> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
import React, { useCallback, useRef } from 'react'; | ||
import Transition, { | ||
TransitionProps, | ||
TransitionStatus, | ||
} from 'react-transition-group/Transition'; | ||
import safeFindDOMNode from 'react-overlays/safeFindDOMNode'; | ||
import useMergedRefs from '@restart/hooks/useMergedRefs'; | ||
|
||
export type TransitionWrapperProps = TransitionProps & { | ||
childRef?: React.Ref<unknown>; | ||
children: | ||
| React.ReactElement | ||
| (( | ||
status: TransitionStatus, | ||
props: Record<string, unknown>, | ||
) => React.ReactNode); | ||
}; | ||
|
||
// Normalizes Transition callbacks when nodeRef is used. | ||
const TransitionWrapper = React.forwardRef< | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can never remember what RTG is doing now...do we need this wrapper? If it's in v5 can we just break the API and require node ref compatible components? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RTG has this quirk where the node is excluded in the callback signature when using nodeRef. ie. This wrapper abstracts all that so callers don't have to worry about it (like they can just use it like it was without using nodeRef). I kinda figured this would be a cleaner approach. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, that seems like a bad API choice i probably ok'd oops |
||
Transition<any>, | ||
TransitionWrapperProps | ||
>( | ||
( | ||
{ | ||
onEnter, | ||
onEntering, | ||
onEntered, | ||
onExit, | ||
onExiting, | ||
onExited, | ||
addEndListener, | ||
children, | ||
childRef, | ||
...props | ||
}, | ||
ref, | ||
) => { | ||
const nodeRef = useRef<HTMLElement>(null); | ||
const mergedRef = useMergedRefs(nodeRef, childRef); | ||
|
||
const attachRef = ( | ||
r: React.ComponentClass | Element | null | undefined, | ||
) => { | ||
mergedRef(safeFindDOMNode(r)); | ||
}; | ||
|
||
const normalize = ( | ||
callback?: (node: HTMLElement, param: unknown) => void, | ||
) => (param: unknown) => { | ||
if (callback && nodeRef.current) { | ||
callback(nodeRef.current, param); | ||
} | ||
}; | ||
|
||
const handleEnter = useCallback(normalize(onEnter), [onEnter]); | ||
const handleEntering = useCallback(normalize(onEntering), [onEntering]); | ||
const handleEntered = useCallback(normalize(onEntered), [onEntered]); | ||
const handleExit = useCallback(normalize(onExit), [onExit]); | ||
const handleExiting = useCallback(normalize(onExiting), [onExiting]); | ||
const handleExited = useCallback(normalize(onExited), [onExited]); | ||
const handleAddEndListener = useCallback(normalize(addEndListener), [ | ||
addEndListener, | ||
]); | ||
|
||
return ( | ||
<Transition | ||
ref={ref} | ||
{...props} | ||
onEnter={handleEnter} | ||
onEntered={handleEntered} | ||
onEntering={handleEntering} | ||
onExit={handleExit} | ||
onExited={handleExited} | ||
onExiting={handleExiting} | ||
addEndListener={handleAddEndListener} | ||
nodeRef={nodeRef} | ||
> | ||
{typeof children === 'function' | ||
? (status: TransitionStatus, innerProps: Record<string, unknown>) => | ||
children(status, { | ||
...innerProps, | ||
ref: attachRef, | ||
}) | ||
: React.cloneElement(children as React.ReactElement, { | ||
ref: attachRef, | ||
})} | ||
</Transition> | ||
); | ||
}, | ||
); | ||
|
||
export default TransitionWrapper; |
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.
this will always be falsey, children, if it's an element would have ref on
children.props
not on the element itselfThere 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.
Hmm
children.ref
is actually defined when I was testing this. I've triedchildren.props.ref
but I get a warning:Warning: div:
ref
is not a prop. Trying to access it will result inundefined
being returned. If you need to access the same value within the child component, you should pass it as a different prop.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.
tbh, children.ref did seem kinda hacky (esp. with the casting). I'm not 100% sure on what the correct approach is :P
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.
o hmm i must have it backwards, been doing this for too long. I'm suprised the types don't correctly cover this tho
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 found this on the react github project that seems to confirm the
children.ref
usage:facebook/react#8873 (comment)