Skip to content
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

Hoverable component refactored from class into functional approach #27223

Merged
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
274 changes: 133 additions & 141 deletions src/components/Hoverable/index.js
Original file line number Diff line number Diff line change
@@ -1,189 +1,181 @@
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';

function mapChildren(children, callbackParam) {
if (_.isArray(children) && children.length === 1) {
return children[0];
}

if (_.isFunction(children)) {
return children(callbackParam);
}

return children;
}

/**
* 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);
function InnerHoverable({disabled, onHoverIn, onHoverOut, children, shouldHandleScroll}, outerRef) {
const [isHovered, setIsHovered] = useState(false);
const [isScrolling, setIsScrolling] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest replacing isScrolling state in favour of isScrollingRef. Since using isScrolling state will re-render the Hoverable component and we can avoid it by using ref, which gives us a similar functionality but with less re-renders.

Screenshot 2023-09-26 at 1 46 12 PM

Copy link
Contributor Author

@kacper-mikolajczak kacper-mikolajczak Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got you @hurali97! I've implemented suggested changes 👍 Let's review it once again and we are good to go :)


this.state = {
isHovered: 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kacper-mikolajczak It seems this listener has been not migrated, checkHover is not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch, thanks! Done ✅


/**
* 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;
});
}
}
isHoveredRef.current = hovered;

componentDidUpdate(prevProps) {
if (prevProps.disabled === this.props.disabled) {
return;
}
if (shouldHandleScroll && isScrolling) {
return;
}
setIsHovered(hovered);
},
[disabled, shouldHandleScroll, isScrolling],
);

if (this.props.disabled && this.state.isHovered) {
this.setState({isHovered: false});
}
}
useEffect(() => {
const unsetHoveredWhenDocumentIsHidden = () => document.visibilityState === 'hidden' && setIsHovered(false);

componentWillUnmount() {
document.removeEventListener('visibilitychange', this.handleVisibilityChange);
document.removeEventListener('mouseover', this.checkHover);
if (this.scrollingListener) {
this.scrollingListener.remove();
}
}

/**
* 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) {
return;
}
document.addEventListener('visibilitychange', unsetHoveredWhenDocumentIsHidden);

/**
* 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.
*/
this.isHoveredRef = isHovered;
return () => document.removeEventListener('visibilitychange', unsetHoveredWhenDocumentIsHidden);
}, []);

/**
* 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) {
useEffect(() => {
if (!shouldHandleScroll) {
return;
}

if (isHovered !== this.state.isHovered) {
this.setState({isHovered}, isHovered ? this.props.onHoverIn : this.props.onHoverOut);
}
}
const scrollingListener = DeviceEventEmitter.addListener(CONST.EVENTS.SCROLLING, (scrolling) => {
setIsScrolling(scrolling);
if (!scrolling) {
setIsHovered(isHoveredRef.current);
}
});

return scrollingListener.remove;
}, [shouldHandleScroll]);

/**
* 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) {
const unsetHoveredIfOutside = useCallback(
(e) => {
if (!ref.current || !isHovered) {
return;
}

if (ref.current.contains(e.target)) {
return;
}

setIsHovered(false);
},
[isHovered],
);

useEffect(() => {
if (!DeviceCapabilities.hasHoverSupport()) {
return;
}

if (this.wrapperView.contains(e.target)) {
return;
}
document.addEventListener('mouseover', unsetHoveredIfOutside);

this.setIsHovered(false);
}
return () => document.removeEventListener('mouseover', unsetHoveredIfOutside);
}, [unsetHoveredIfOutside]);

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) => {
this.setIsHovered(true);

if (_.isFunction(child.props.onMouseEnter)) {
child.props.onMouseEnter(el);
}
},
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]);

useImperativeHandle(outerRef, () => ref.current, []);
Beamanator marked this conversation as resolved.
Show resolved Hide resolved

const child = useMemo(() => React.Children.only(mapChildren(children, isHovered)), [children, isHovered]);

const onMouseEnter = useCallback(
(el) => {
updateIsHoveredOnScrolling(true);

if (_.isFunction(child.props.onMouseEnter)) {
child.props.onMouseEnter(el);
}
},
[child.props, updateIsHoveredOnScrolling],
);

const onMouseLeave = useCallback(
(el) => {
updateIsHoveredOnScrolling(false);

if (_.isFunction(child.props.onMouseLeave)) {
child.props.onMouseLeave(el);
}
},
[child.props, updateIsHoveredOnScrolling],
);

const onBlur = useCallback(
(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 (!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,
onMouseEnter,
onMouseLeave,
onBlur,
});
}

const Hoverable = React.forwardRef(InnerHoverable);

Hoverable.propTypes = propTypes;
Hoverable.defaultProps = defaultProps;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a displayName field

Hoverable.displayName = 'Hoverable';

Copy link
Contributor Author

@kacper-mikolajczak kacper-mikolajczak Sep 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, there is no need to set displayName explicitly on components. Should we propose an update to the style doc you've mentioned? Thanks for noticing that!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but this standard is from Expensify codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let's keep it this way then :)

Hoverable.displayName = 'Hoverable';

export default Hoverable;
Loading