-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Refactor Hoverable component into functional #29052
Merged
Beamanator
merged 11 commits into
Expensify:main
from
kacper-mikolajczak:refactor/16161/hoverable-class-to-functional
Oct 17, 2023
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
a839a64
fix refs
kacper-mikolajczak 95ce7ae
fix Tooltip memoization
kacper-mikolajczak 70999e7
Merge branch 'main' into refactor/16161/hoverable-class-to-functional
kacper-mikolajczak c9eb5a7
obey the forwardRef tips
kacper-mikolajczak 16cabc5
fix Tooltip and BaseTooltip as wrong file was changed
kacper-mikolajczak b5f9a86
remove unnecessary memo in Tooltip
kacper-mikolajczak 5461a71
add clarification comments
kacper-mikolajczak 0246282
Add meaningful names to event callbacks
kacper-mikolajczak a4a4ea0
Merge branch 'main' into refactor/16161/hoverable-class-to-functional
kacper-mikolajczak 599b119
incorporate unsetHoveredIfOutside into useEffect
kacper-mikolajczak c585d31
Merge branch 'main' into refactor/16161/hoverable-class-to-functional
kacper-mikolajczak 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 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,197 +1,216 @@ | ||
import _ from 'underscore'; | ||
import React, {Component} from 'react'; | ||
import React, {useEffect, useCallback, useState, useRef, useMemo, useImperativeHandle} from 'react'; | ||
import {DeviceEventEmitter} from 'react-native'; | ||
import {propTypes, defaultProps} from './hoverablePropTypes'; | ||
import * as DeviceCapabilities from '../../libs/DeviceCapabilities'; | ||
import CONST from '../../CONST'; | ||
|
||
/** | ||
* Maps the children of a Hoverable component to | ||
* - a function that is called with the parameter | ||
* - the child itself if it is the only child | ||
* @param {Array|Function|ReactNode} children - The children to map. | ||
* @param {Object} callbackParam - The parameter to pass to the children function. | ||
* @returns {ReactNode} The mapped children. | ||
*/ | ||
function mapChildren(children, callbackParam) { | ||
if (_.isArray(children) && children.length === 1) { | ||
return children[0]; | ||
} | ||
|
||
if (_.isFunction(children)) { | ||
return children(callbackParam); | ||
} | ||
|
||
return children; | ||
} | ||
|
||
/** | ||
* Assigns a ref to an element, either by setting the current property of the ref object or by calling the ref function | ||
* @param {Object|Function} ref - The ref object or function. | ||
* @param {HTMLElement} el - The element to assign the ref to. | ||
*/ | ||
function assignRef(ref, el) { | ||
if (!ref) { | ||
return; | ||
} | ||
|
||
if (_.has(ref, 'current')) { | ||
// eslint-disable-next-line no-param-reassign | ||
ref.current = el; | ||
} | ||
|
||
if (_.isFunction(ref)) { | ||
ref(el); | ||
} | ||
} | ||
|
||
/** | ||
* It is necessary to create a Hoverable component instead of relying solely on Pressable support for hover state, | ||
* because nesting Pressables causes issues where the hovered state of the child cannot be easily propagated to the | ||
* parent. https://github.com/necolas/react-native-web/issues/1875 | ||
*/ | ||
class Hoverable extends Component { | ||
constructor(props) { | ||
super(props); | ||
|
||
this.handleVisibilityChange = this.handleVisibilityChange.bind(this); | ||
this.checkHover = this.checkHover.bind(this); | ||
const Hoverable = React.forwardRef(({disabled, onHoverIn, onHoverOut, onMouseEnter, onMouseLeave, children, shouldHandleScroll}, outerRef) => { | ||
const [isHovered, setIsHovered] = useState(false); | ||
|
||
this.state = { | ||
isHovered: false, | ||
}; | ||
const isScrolling = useRef(false); | ||
const isHoveredRef = useRef(false); | ||
const ref = useRef(null); | ||
|
||
this.isHoveredRef = false; | ||
this.isScrollingRef = false; | ||
this.wrapperView = null; | ||
} | ||
const updateIsHoveredOnScrolling = useCallback( | ||
(hovered) => { | ||
if (disabled) { | ||
return; | ||
} | ||
|
||
componentDidMount() { | ||
document.addEventListener('visibilitychange', this.handleVisibilityChange); | ||
document.addEventListener('mouseover', this.checkHover); | ||
isHoveredRef.current = hovered; | ||
|
||
/** | ||
* Only add the scrolling listener if the shouldHandleScroll prop is true | ||
* and the scrollingListener is not already set. | ||
*/ | ||
if (!this.scrollingListener && this.props.shouldHandleScroll) { | ||
this.scrollingListener = DeviceEventEmitter.addListener(CONST.EVENTS.SCROLLING, (scrolling) => { | ||
/** | ||
* If user has stopped scrolling and the isHoveredRef is true, then we should update the hover state. | ||
*/ | ||
if (!scrolling && this.isHoveredRef) { | ||
this.setState({isHovered: this.isHoveredRef}, this.props.onHoverIn); | ||
} else if (scrolling && this.isHoveredRef) { | ||
/** | ||
* If the user has started scrolling and the isHoveredRef is true, then we should set the hover state to false. | ||
* This is to hide the existing hover and reaction bar. | ||
*/ | ||
this.setState({isHovered: false}, this.props.onHoverOut); | ||
} | ||
this.isScrollingRef = scrolling; | ||
}); | ||
} | ||
} | ||
if (shouldHandleScroll && isScrolling.current) { | ||
return; | ||
} | ||
setIsHovered(hovered); | ||
}, | ||
[disabled, shouldHandleScroll], | ||
); | ||
|
||
useEffect(() => { | ||
const unsetHoveredWhenDocumentIsHidden = () => document.visibilityState === 'hidden' && setIsHovered(false); | ||
|
||
document.addEventListener('visibilitychange', unsetHoveredWhenDocumentIsHidden); | ||
|
||
componentDidUpdate(prevProps) { | ||
if (prevProps.disabled === this.props.disabled) { | ||
return () => document.removeEventListener('visibilitychange', unsetHoveredWhenDocumentIsHidden); | ||
}, []); | ||
|
||
useEffect(() => { | ||
if (!shouldHandleScroll) { | ||
return; | ||
} | ||
|
||
if (this.props.disabled && this.state.isHovered) { | ||
this.setState({isHovered: false}); | ||
} | ||
} | ||
const scrollingListener = DeviceEventEmitter.addListener(CONST.EVENTS.SCROLLING, (scrolling) => { | ||
isScrolling.current = scrolling; | ||
if (!scrolling) { | ||
setIsHovered(isHoveredRef.current); | ||
} | ||
}); | ||
|
||
componentWillUnmount() { | ||
document.removeEventListener('visibilitychange', this.handleVisibilityChange); | ||
document.removeEventListener('mouseover', this.checkHover); | ||
if (this.scrollingListener) { | ||
this.scrollingListener.remove(); | ||
} | ||
} | ||
return () => scrollingListener.remove(); | ||
}, [shouldHandleScroll]); | ||
|
||
/** | ||
* Sets the hover state of this component to true and execute the onHoverIn callback. | ||
* | ||
* @param {Boolean} isHovered - Whether or not this component is hovered. | ||
*/ | ||
setIsHovered(isHovered) { | ||
if (this.props.disabled) { | ||
useEffect(() => { | ||
if (!DeviceCapabilities.hasHoverSupport()) { | ||
return; | ||
} | ||
|
||
/** | ||
* Capture whther or not the user is hovering over the component. | ||
* We will use this to determine if we should update the hover state when the user has stopped scrolling. | ||
* Checks the hover state of a component and updates it based on the event target. | ||
* This is necessary to handle cases where the hover state might get stuck due to an unreliable mouseleave trigger, | ||
* such as when an element is removed before the mouseleave event is triggered. | ||
* @param {Event} e - The hover event object. | ||
*/ | ||
this.isHoveredRef = isHovered; | ||
const unsetHoveredIfOutside = (e) => { | ||
if (!ref.current || !isHovered) { | ||
return; | ||
} | ||
|
||
/** | ||
* If the isScrollingRef is true, then the user is scrolling and we should not update the hover state. | ||
*/ | ||
if (this.isScrollingRef && this.props.shouldHandleScroll && !this.state.isHovered) { | ||
return; | ||
} | ||
if (ref.current.contains(e.target)) { | ||
return; | ||
} | ||
|
||
if (isHovered !== this.state.isHovered) { | ||
this.setState({isHovered}, isHovered ? this.props.onHoverIn : this.props.onHoverOut); | ||
} | ||
} | ||
setIsHovered(false); | ||
}; | ||
|
||
/** | ||
* Checks the hover state of a component and updates it based on the event target. | ||
* This is necessary to handle cases where the hover state might get stuck due to an unreliable mouseleave trigger, | ||
* such as when an element is removed before the mouseleave event is triggered. | ||
* @param {Event} e - The hover event object. | ||
*/ | ||
checkHover(e) { | ||
if (!this.wrapperView || !this.state.isHovered) { | ||
return; | ||
} | ||
document.addEventListener('mouseover', unsetHoveredIfOutside); | ||
|
||
if (this.wrapperView.contains(e.target)) { | ||
return; | ||
} | ||
|
||
this.setIsHovered(false); | ||
} | ||
return () => document.removeEventListener('mouseover', unsetHoveredIfOutside); | ||
}, [isHovered]); | ||
|
||
handleVisibilityChange() { | ||
if (document.visibilityState !== 'hidden') { | ||
useEffect(() => { | ||
if (!disabled || !isHovered) { | ||
return; | ||
} | ||
setIsHovered(false); | ||
}, [disabled, isHovered]); | ||
|
||
this.setIsHovered(false); | ||
} | ||
|
||
render() { | ||
let child = this.props.children; | ||
if (_.isArray(this.props.children) && this.props.children.length === 1) { | ||
child = this.props.children[0]; | ||
useEffect(() => { | ||
if (disabled) { | ||
return; | ||
} | ||
|
||
if (_.isFunction(child)) { | ||
child = child(this.state.isHovered); | ||
if (onHoverIn && isHovered) { | ||
return onHoverIn(); | ||
} | ||
|
||
if (!DeviceCapabilities.hasHoverSupport()) { | ||
return child; | ||
if (onHoverOut && !isHovered) { | ||
return onHoverOut(); | ||
} | ||
|
||
return React.cloneElement(React.Children.only(child), { | ||
ref: (el) => { | ||
this.wrapperView = el; | ||
|
||
// Call the original ref, if any | ||
const {ref} = child; | ||
if (_.isFunction(ref)) { | ||
ref(el); | ||
return; | ||
} | ||
|
||
if (_.isObject(ref)) { | ||
ref.current = el; | ||
} | ||
}, | ||
onMouseEnter: (el) => { | ||
if (_.isFunction(this.props.onMouseEnter)) { | ||
this.props.onMouseEnter(el); | ||
} | ||
|
||
this.setIsHovered(true); | ||
|
||
if (_.isFunction(child.props.onMouseEnter)) { | ||
child.props.onMouseEnter(el); | ||
} | ||
}, | ||
onMouseLeave: (el) => { | ||
if (_.isFunction(this.props.onMouseLeave)) { | ||
this.props.onMouseLeave(el); | ||
} | ||
|
||
this.setIsHovered(false); | ||
|
||
if (_.isFunction(child.props.onMouseLeave)) { | ||
child.props.onMouseLeave(el); | ||
} | ||
}, | ||
onBlur: (el) => { | ||
// Check if the blur event occurred due to clicking outside the element | ||
// and the wrapperView contains the element that caused the blur and reset isHovered | ||
if (!this.wrapperView.contains(el.target) && !this.wrapperView.contains(el.relatedTarget)) { | ||
this.setIsHovered(false); | ||
} | ||
|
||
if (_.isFunction(child.props.onBlur)) { | ||
child.props.onBlur(el); | ||
} | ||
}, | ||
}); | ||
}, [disabled, isHovered, onHoverIn, onHoverOut]); | ||
|
||
// Expose inner ref to parent through outerRef. This enable us to use ref both in parent and child. | ||
useImperativeHandle(outerRef, () => ref.current, []); | ||
|
||
const child = useMemo(() => React.Children.only(mapChildren(children, isHovered)), [children, isHovered]); | ||
|
||
const enableHoveredOnMouseEnter = useCallback( | ||
(el) => { | ||
kacper-mikolajczak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
updateIsHoveredOnScrolling(true); | ||
|
||
if (_.isFunction(onMouseEnter)) { | ||
onMouseEnter(el); | ||
} | ||
|
||
if (_.isFunction(child.props.onMouseEnter)) { | ||
child.props.onMouseEnter(el); | ||
} | ||
}, | ||
[child.props, onMouseEnter, updateIsHoveredOnScrolling], | ||
); | ||
|
||
const disableHoveredOnMouseLeave = useCallback( | ||
(el) => { | ||
kacper-mikolajczak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
updateIsHoveredOnScrolling(false); | ||
|
||
if (_.isFunction(onMouseLeave)) { | ||
onMouseLeave(el); | ||
} | ||
|
||
if (_.isFunction(child.props.onMouseLeave)) { | ||
child.props.onMouseLeave(el); | ||
} | ||
}, | ||
[child.props, onMouseLeave, updateIsHoveredOnScrolling], | ||
); | ||
|
||
const disableHoveredOnBlur = useCallback( | ||
(el) => { | ||
kacper-mikolajczak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Check if the blur event occurred due to clicking outside the element | ||
// and the wrapperView contains the element that caused the blur and reset isHovered | ||
if (!ref.current.contains(el.target) && !ref.current.contains(el.relatedTarget)) { | ||
setIsHovered(false); | ||
} | ||
|
||
if (_.isFunction(child.props.onBlur)) { | ||
child.props.onBlur(el); | ||
} | ||
}, | ||
[child.props], | ||
); | ||
|
||
if (!DeviceCapabilities.hasHoverSupport()) { | ||
return child; | ||
} | ||
} | ||
|
||
return React.cloneElement(child, { | ||
ref: (el) => { | ||
ref.current = el; | ||
assignRef(child.ref, el); | ||
}, | ||
onMouseEnter: enableHoveredOnMouseEnter, | ||
onMouseLeave: disableHoveredOnMouseLeave, | ||
onBlur: disableHoveredOnBlur, | ||
}); | ||
}); | ||
|
||
Hoverable.propTypes = propTypes; | ||
Hoverable.defaultProps = defaultProps; | ||
Hoverable.displayName = 'Hoverable'; | ||
|
||
export default Hoverable; |
Oops, something went wrong.
Oops, something went wrong.
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.
I think about adding comments for
mapChildren
andassignRef
to clarify why we have this logic.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.
Thanks for the idea, done ✅
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.
Thank you, looks good!
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.
@kacper-mikolajczak, do we need to extract this logic outside of the component? Usually, if logic is extracted should be moved to utils or helpers dir
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.
@ArekChr Those helpers are not really that universal to move them into utils - I did not want to bloat the component body no more, thus extracted them away. This might help to reduce the cognitive load when studying the component, wdyt?
If not, is there a convention we use for such local helpers? Shall we create a separate file under component's directory called
utils
/helpers
?Thanks for pointing this out!
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 agree that local functions simplify readability. For me, it can stay as it is. But
assignRef
seems to be a universal function. It could be moved to, e.g. react utils or helpers. @Beamanator, any thoughts?