From 24381ed092b0ec280083b9454fd6968710e552fd Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Thu, 16 Mar 2023 10:34:31 -0700 Subject: [PATCH 01/18] refactor tooltip to fix double focusable component --- .../BaseAnchorForCommentsOnly.js | 2 +- src/components/AvatarWithIndicator.js | 8 +- src/components/DisplayNames/index.js | 1 - .../EmojiPicker/CategoryShortcutButton.js | 29 +++-- src/components/FloatingActionButton.js | 2 +- src/components/Hoverable/index.js | 103 ++++++++---------- src/components/MultipleAvatars.js | 14 +-- src/components/OptionRow.js | 6 +- src/components/Reactions/AddReactionBubble.js | 2 +- .../BaseQuickEmojiReactions.js | 4 +- src/components/ReportWelcomeText.js | 2 +- src/components/SubscriptAvatar.js | 32 +++--- src/components/Tooltip/index.js | 72 ++++++------ .../home/report/ReportActionItemSingle.js | 18 +-- src/pages/settings/InitialSettingsPage.js | 9 +- src/pages/signin/SignInPageLayout/Footer.js | 15 ++- src/pages/workspace/WorkspaceInitialPage.js | 16 +-- src/stories/Tooltip.stories.js | 1 - 18 files changed, 160 insertions(+), 176 deletions(-) diff --git a/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js b/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js index 18d3fbf25960..22a4ce75f99c 100644 --- a/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js +++ b/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly.js @@ -65,7 +65,7 @@ const BaseAnchorForCommentsOnly = (props) => { onPressIn={props.onPressIn} onPressOut={props.onPressOut} > - + linkRef = el} style={StyleSheet.flatten([props.style, defaultTextStyle])} diff --git a/src/components/AvatarWithIndicator.js b/src/components/AvatarWithIndicator.js index 6430d463a712..9bae6d9488c6 100644 --- a/src/components/AvatarWithIndicator.js +++ b/src/components/AvatarWithIndicator.js @@ -85,8 +85,8 @@ const AvatarWithIndicator = (props) => { const shouldShowIndicator = _.some(errorCheckingMethods, errorCheckingMethod => errorCheckingMethod()); return ( - - + + { {shouldShowIndicator && ( )} - - + + ); }; diff --git a/src/components/DisplayNames/index.js b/src/components/DisplayNames/index.js index fd23a011bf69..83def9ce8c05 100644 --- a/src/components/DisplayNames/index.js +++ b/src/components/DisplayNames/index.js @@ -95,7 +95,6 @@ class DisplayNames extends PureComponent { this.getTooltipShiftX(index)} > {/* // We need to get the refs to all the names which will be used to correct diff --git a/src/components/EmojiPicker/CategoryShortcutButton.js b/src/components/EmojiPicker/CategoryShortcutButton.js index 2c5f061327ae..33d5576c29ea 100644 --- a/src/components/EmojiPicker/CategoryShortcutButton.js +++ b/src/components/EmojiPicker/CategoryShortcutButton.js @@ -33,20 +33,19 @@ class CategoryShortcutButton extends PureComponent { render() { return ( - this.setState({isHighlighted: true})} - onHoverOut={() => this.setState({isHighlighted: false})} - style={({pressed}) => ([ - StyleUtils.getButtonBackgroundColorStyle(getButtonState(false, pressed)), - styles.categoryShortcutButton, - this.state.isHighlighted && styles.emojiItemHighlighted, - ])} + - this.setState({isHighlighted: true})} + onHoverOut={() => this.setState({isHighlighted: false})} + style={({pressed}) => ([ + StyleUtils.getButtonBackgroundColorStyle(getButtonState(false, pressed)), + styles.categoryShortcutButton, + this.state.isHighlighted && styles.emojiItemHighlighted, + ])} > - - + + ); } } diff --git a/src/components/FloatingActionButton.js b/src/components/FloatingActionButton.js index 2bc1d96d05ec..13275e5116a3 100644 --- a/src/components/FloatingActionButton.js +++ b/src/components/FloatingActionButton.js @@ -73,7 +73,7 @@ class FloatingActionButton extends PureComponent { }); return ( - + this.fabPressable = el} diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js index 2f10cca0c1e3..2f275dd968fe 100644 --- a/src/components/Hoverable/index.js +++ b/src/components/Hoverable/index.js @@ -55,68 +55,55 @@ class Hoverable extends Component { } render() { - if (this.props.absolute && React.isValidElement(this.props.children)) { - return React.cloneElement(React.Children.only(this.props.children), { - ref: (el) => { - this.wrapperView = el; - - // Call the original ref, if any - const {ref} = this.props.children; - if (_.isFunction(ref)) { - ref(el); - } - }, - onMouseEnter: (el) => { - this.setIsHovered(true); - - // Call the original onMouseEnter, if any - const {onMouseEnter} = this.props.children; - if (_.isFunction(onMouseEnter)) { - onMouseEnter(el); - } - }, - onMouseLeave: (el) => { - this.setIsHovered(false); + const child = _.isFunction(this.props.children) + ? this.props.children(this.state.isHovered) + : this.props.children - // Call the original onMouseLeave, if any - const {onMouseLeave} = this.props.children; - if (_.isFunction(onMouseLeave)) { - onMouseLeave(el); - } - }, - onBlur: (el) => { - if (!this.wrapperView.contains(el.relatedTarget)) { - this.setIsHovered(false); - } - - // Call the original onBlur, if any - const {onBlur} = this.props.children; - if (_.isFunction(onBlur)) { - onBlur(el); - } - }, - }); + if (!React.isValidElement(child)) { + throw Error("Children is not a valid element."); } - return ( - this.wrapperView = el} - onMouseEnter={() => this.setIsHovered(true)} - onMouseLeave={() => this.setIsHovered(false)} - onBlur={(el) => { - if (this.wrapperView.contains(el.relatedTarget)) { - return; - } + + 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); + } + }, + onMouseEnter: (el) => { + console.log("ENTER") + this.setIsHovered(true); + + // Call the original onMouseEnter, if any + const {onMouseEnter} = child; + if (_.isFunction(onMouseEnter)) { + onMouseEnter(el); + } + }, + onMouseLeave: (el) => { + this.setIsHovered(false); + + // Call the original onMouseLeave, if any + const {onMouseLeave} = child; + if (_.isFunction(onMouseLeave)) { + onMouseLeave(el); + } + }, + onBlur: (el) => { + if (!this.wrapperView.contains(el.relatedTarget)) { this.setIsHovered(false); - }} - > - { // If this.props.children is a function, call it to provide the hover state to the children. - _.isFunction(this.props.children) - ? this.props.children(this.state.isHovered) - : this.props.children } - - ); + + // Call the original onBlur, if any + const {onBlur} = child; + if (_.isFunction(onBlur)) { + onBlur(el); + } + }, + }); } } diff --git a/src/components/MultipleAvatars.js b/src/components/MultipleAvatars.js index 6e30a2579a25..9a5099483fcc 100644 --- a/src/components/MultipleAvatars.js +++ b/src/components/MultipleAvatars.js @@ -69,8 +69,8 @@ const MultipleAvatars = (props) => { if (props.icons.length === 1 && !props.shouldStackHorizontally) { return ( - - + + { name={props.icons[0].name} type={props.icons[0].type} /> - - + + ); } @@ -127,7 +127,7 @@ const MultipleAvatars = (props) => { - + {/* View is necessary for tooltip to show for multiple avatars in LHN */} { style={secondAvatarStyles} > {props.icons.length === 2 ? ( - + { ) : ( - + diff --git a/src/components/OptionRow.js b/src/components/OptionRow.js index 532ae783bdd6..1bf7219df9d2 100644 --- a/src/components/OptionRow.js +++ b/src/components/OptionRow.js @@ -149,11 +149,7 @@ class OptionRow extends Component { errors={this.props.option.allReportErrors} shouldShowErrorMessages={false} > - + {hovered => ( touchableRef = el} diff --git a/src/components/Reactions/AddReactionBubble.js b/src/components/Reactions/AddReactionBubble.js index 3b57771311ab..0eb40ecccdc7 100644 --- a/src/components/Reactions/AddReactionBubble.js +++ b/src/components/Reactions/AddReactionBubble.js @@ -79,7 +79,7 @@ const AddReactionBubble = (props) => { }; return ( - + ( {_.map(CONST.QUICK_REACTIONS, emoji => ( - - // Note: focus is handled by the Pressable component in EmojiReactionBubble - + { displayName, pronouns, tooltip, }, index) => ( - + Navigation.navigate(ROUTES.getDetailsRoute(participants[index]))}> {displayName} diff --git a/src/components/SubscriptAvatar.js b/src/components/SubscriptAvatar.js index 6da3e918fec4..44b3eb723671 100644 --- a/src/components/SubscriptAvatar.js +++ b/src/components/SubscriptAvatar.js @@ -42,20 +42,22 @@ const defaultProps = { const SubscriptAvatar = props => ( - + + + - - + + ( name={props.secondaryAvatar.name} type={props.secondaryAvatar.type} /> - - + + ); diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index 104098b375f0..cb4c8b676759 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -1,6 +1,6 @@ import _ from 'underscore'; import React, {PureComponent} from 'react'; -import {Animated, View} from 'react-native'; +import {Animated} from 'react-native'; import TooltipRenderedOnPageBody from './TooltipRenderedOnPageBody'; import Hoverable from '../Hoverable'; import withWindowDimensions from '../withWindowDimensions'; @@ -37,6 +37,7 @@ class Tooltip extends PureComponent { this.getWrapperPosition = this.getWrapperPosition.bind(this); this.showTooltip = this.showTooltip.bind(this); this.hideTooltip = this.hideTooltip.bind(this); + this.isFocusable = this.isFocusable.bind(this); } componentDidUpdate(prevProps) { @@ -141,47 +142,50 @@ class Tooltip extends PureComponent { TooltipSense.deactivate(); } + /** + * Returns true if children is a focusable component. + * + * @returns {Boolean} + */ + isFocusable() { + const name = this.props.children.type.displayName; + const isPressableText = name === 'Text' && Boolean(this.props.children.props.onPress); + return isPressableText || ['TouchableOpacity', 'Pressable', 'TouchableWithoutFeedback'].includes(name); + } + render() { // Skip the tooltip and return the children if the text is empty, // we don't have a render function or the device does not support hovering if ((_.isEmpty(this.props.text) && this.props.renderTooltipContent == null) || !this.hasHoverSupport) { return this.props.children; } - let child = ( - this.wrapperView = el} - onBlur={this.hideTooltip} - focusable={this.props.focusable} - style={this.props.containerStyles} - > - {this.props.children} - - ); - - if (this.props.absolute && React.isValidElement(this.props.children)) { - child = React.cloneElement(React.Children.only(this.props.children), { - ref: (el) => { - this.wrapperView = el; - // Call the original ref, if any - const {ref} = this.props.children; - if (_.isFunction(ref)) { - ref(el); - } - }, - onBlur: (el) => { - this.hideTooltip(); - - // Call the original onBlur, if any - const {onBlur} = this.props.children; - if (_.isFunction(onBlur)) { - onBlur(el); - } - }, - focusable: true, - }); + if (!React.isValidElement(this.props.children)) { + throw Error("Children is not a valid element."); } + const child = React.cloneElement(React.Children.only(this.props.children), { + ref: (el) => { + this.wrapperView = el; + + // Call the original ref, if any + const {ref} = this.props.children; + if (_.isFunction(ref)) { + ref(el); + } + }, + onBlur: (el) => { + this.hideTooltip(); + + // Call the original onBlur, if any + const {onBlur} = this.props.children; + if (_.isFunction(onBlur)) { + onBlur(el); + } + }, + focusable: this.isFocusable(), + }); + return ( <> {this.state.isRendered && ( @@ -201,8 +205,6 @@ class Tooltip extends PureComponent { /> )} diff --git a/src/pages/home/report/ReportActionItemSingle.js b/src/pages/home/report/ReportActionItemSingle.js index 19d2cc05f58c..d6f5207e5a91 100644 --- a/src/pages/home/report/ReportActionItemSingle.js +++ b/src/pages/home/report/ReportActionItemSingle.js @@ -70,13 +70,13 @@ const ReportActionItemSingle = (props) => { return ( - showUserDetails(actorEmail)} - > - + + showUserDetails(actorEmail)} + > @@ -85,8 +85,8 @@ const ReportActionItemSingle = (props) => { source={avatarSource} /> - - + + {props.showHeader ? ( diff --git a/src/pages/settings/InitialSettingsPage.js b/src/pages/settings/InitialSettingsPage.js index 6d6db5a498f3..16cae58efe31 100755 --- a/src/pages/settings/InitialSettingsPage.js +++ b/src/pages/settings/InitialSettingsPage.js @@ -258,8 +258,8 @@ class InitialSettingsPage extends React.Component { - - + + @@ -269,9 +269,8 @@ class InitialSettingsPage extends React.Component { size={CONST.AVATAR_SIZE.LARGE} /> - - - + + diff --git a/src/pages/signin/SignInPageLayout/Footer.js b/src/pages/signin/SignInPageLayout/Footer.js index 0f6e099bc8c0..2b0f9cd7cf60 100644 --- a/src/pages/signin/SignInPageLayout/Footer.js +++ b/src/pages/signin/SignInPageLayout/Footer.js @@ -162,12 +162,15 @@ const Footer = (props) => { key={row.translationPath} > {hovered => ( - - {props.translate(row.translationPath)} - + + + {console.log(hovered)} + {props.translate(row.translationPath)} + + )} ))} diff --git a/src/pages/workspace/WorkspaceInitialPage.js b/src/pages/workspace/WorkspaceInitialPage.js index fea68a5af262..f4c071457e11 100644 --- a/src/pages/workspace/WorkspaceInitialPage.js +++ b/src/pages/workspace/WorkspaceInitialPage.js @@ -187,12 +187,12 @@ class WorkspaceInitialPage extends React.Component { - - + + - - + + {!_.isEmpty(this.props.policy.name) && ( { From bd13ee31669132ce80e7a3f77fe49bff209d9f2e Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Thu, 16 Mar 2023 10:47:32 -0700 Subject: [PATCH 02/18] remove console log --- src/components/Hoverable/index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js index 2f275dd968fe..40574e863e92 100644 --- a/src/components/Hoverable/index.js +++ b/src/components/Hoverable/index.js @@ -74,7 +74,6 @@ class Hoverable extends Component { } }, onMouseEnter: (el) => { - console.log("ENTER") this.setIsHovered(true); // Call the original onMouseEnter, if any From 79031f1d341db711f2eee8df30beb7402b7460a3 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Thu, 16 Mar 2023 10:50:58 -0700 Subject: [PATCH 03/18] fix lint --- src/components/Hoverable/index.js | 5 ++--- src/components/Tooltip/index.js | 2 +- src/pages/signin/SignInPageLayout/Footer.js | 1 - 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js index 40574e863e92..7eed07748ed1 100644 --- a/src/components/Hoverable/index.js +++ b/src/components/Hoverable/index.js @@ -1,6 +1,5 @@ import _ from 'underscore'; import React, {Component} from 'react'; -import {View} from 'react-native'; import {propTypes, defaultProps} from './hoverablePropTypes'; /** @@ -57,10 +56,10 @@ class Hoverable extends Component { render() { const child = _.isFunction(this.props.children) ? this.props.children(this.state.isHovered) - : this.props.children + : this.props.children; if (!React.isValidElement(child)) { - throw Error("Children is not a valid element."); + throw Error('Children is not a valid element.'); } return React.cloneElement(React.Children.only(child), { diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index cb4c8b676759..b3c555db5da0 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -161,7 +161,7 @@ class Tooltip extends PureComponent { } if (!React.isValidElement(this.props.children)) { - throw Error("Children is not a valid element."); + throw Error('Children is not a valid element.'); } const child = React.cloneElement(React.Children.only(this.props.children), { diff --git a/src/pages/signin/SignInPageLayout/Footer.js b/src/pages/signin/SignInPageLayout/Footer.js index 2b0f9cd7cf60..3e522b0ce315 100644 --- a/src/pages/signin/SignInPageLayout/Footer.js +++ b/src/pages/signin/SignInPageLayout/Footer.js @@ -167,7 +167,6 @@ const Footer = (props) => { style={[styles.footerRow, hovered ? styles.textBlue : {}]} href={row.link} > - {console.log(hovered)} {props.translate(row.translationPath)} From 1d72fc3055f7df79311fdfd3fc18c4352148c61f Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Thu, 16 Mar 2023 10:52:32 -0700 Subject: [PATCH 04/18] remove absolute prop --- src/components/Tooltip/tooltipPropTypes.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/components/Tooltip/tooltipPropTypes.js b/src/components/Tooltip/tooltipPropTypes.js index 014bdc3354b2..0bdf43971c00 100644 --- a/src/components/Tooltip/tooltipPropTypes.js +++ b/src/components/Tooltip/tooltipPropTypes.js @@ -4,9 +4,6 @@ import variables from '../../styles/variables'; import CONST from '../../CONST'; const propTypes = { - /** Enable support for the absolute positioned native(View|Text) children. It will only work for single native child */ - absolute: PropTypes.bool, - /** The text to display in the tooltip. */ text: PropTypes.string, @@ -41,7 +38,6 @@ const propTypes = { }; const defaultProps = { - absolute: false, shiftHorizontal: 0, shiftVertical: 0, containerStyles: [], From 0f7ed5451e4a067beb3848c347838ea574aa701e Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Thu, 16 Mar 2023 10:52:54 -0700 Subject: [PATCH 05/18] remove focusable prop --- src/components/Tooltip/tooltipPropTypes.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/components/Tooltip/tooltipPropTypes.js b/src/components/Tooltip/tooltipPropTypes.js index 0bdf43971c00..2802bb18ec11 100644 --- a/src/components/Tooltip/tooltipPropTypes.js +++ b/src/components/Tooltip/tooltipPropTypes.js @@ -30,9 +30,6 @@ const propTypes = { /** Number of pixels to set max-width on tooltip */ maxWidth: PropTypes.number, - /** Accessibility prop. Sets the tabindex to 0 if true. Default is true. */ - focusable: PropTypes.bool, - /** Render custom content inside the tooltip. Note: This cannot be used together with the text props. */ renderTooltipContent: PropTypes.func, }; @@ -45,7 +42,6 @@ const defaultProps = { maxWidth: variables.sideBarWidth, numberOfLines: CONST.TOOLTIP_MAX_LINES, renderTooltipContent: undefined, - focusable: true, }; export { From a0090b625e380690b87e417126245e6e35b21693 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Fri, 14 Apr 2023 14:15:57 +0800 Subject: [PATCH 06/18] remove focusable and onblur --- src/components/Tooltip/index.js | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index b3c555db5da0..87719910f21c 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -37,7 +37,6 @@ class Tooltip extends PureComponent { this.getWrapperPosition = this.getWrapperPosition.bind(this); this.showTooltip = this.showTooltip.bind(this); this.hideTooltip = this.hideTooltip.bind(this); - this.isFocusable = this.isFocusable.bind(this); } componentDidUpdate(prevProps) { @@ -142,17 +141,6 @@ class Tooltip extends PureComponent { TooltipSense.deactivate(); } - /** - * Returns true if children is a focusable component. - * - * @returns {Boolean} - */ - isFocusable() { - const name = this.props.children.type.displayName; - const isPressableText = name === 'Text' && Boolean(this.props.children.props.onPress); - return isPressableText || ['TouchableOpacity', 'Pressable', 'TouchableWithoutFeedback'].includes(name); - } - render() { // Skip the tooltip and return the children if the text is empty, // we don't have a render function or the device does not support hovering @@ -174,16 +162,6 @@ class Tooltip extends PureComponent { ref(el); } }, - onBlur: (el) => { - this.hideTooltip(); - - // Call the original onBlur, if any - const {onBlur} = this.props.children; - if (_.isFunction(onBlur)) { - onBlur(el); - } - }, - focusable: this.isFocusable(), }); return ( From c8cf0d26e69a40d56e61e7d5badc385ab48d5d57 Mon Sep 17 00:00:00 2001 From: Bernhard Owen Josephus Date: Fri, 14 Apr 2023 14:18:05 +0800 Subject: [PATCH 07/18] revert wrong change --- src/components/Reactions/AddReactionBubble.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Reactions/AddReactionBubble.js b/src/components/Reactions/AddReactionBubble.js index cc19b41dc2a7..0053da3ba6e7 100644 --- a/src/components/Reactions/AddReactionBubble.js +++ b/src/components/Reactions/AddReactionBubble.js @@ -66,7 +66,7 @@ const AddReactionBubble = (props) => { }; return ( - + Date: Fri, 14 Apr 2023 17:27:38 +0800 Subject: [PATCH 08/18] refactor tooltip usage --- src/components/AttachmentCarousel/index.js | 16 +++++----- src/components/AttachmentView.js | 8 ++--- .../AvatarCropModal/AvatarCropModal.js | 18 ++++++----- src/components/AvatarWithImagePicker.js | 32 ++++++++++--------- src/components/CopyTextToClipboard.js | 17 ++++++---- .../EmojiPicker/EmojiPickerButton.js | 2 +- .../BaseQuickEmojiReactions.js | 16 ++++++---- src/pages/home/HeaderView.js | 4 ++- src/pages/workspace/WorkspaceInitialPage.js | 1 - 9 files changed, 62 insertions(+), 52 deletions(-) diff --git a/src/components/AttachmentCarousel/index.js b/src/components/AttachmentCarousel/index.js index c2caebcc07a2..1993032cdc79 100644 --- a/src/components/AttachmentCarousel/index.js +++ b/src/components/AttachmentCarousel/index.js @@ -168,8 +168,8 @@ class AttachmentCarousel extends React.Component { {(isPageSet && this.state.shouldShowArrow) && ( <> {!this.state.isBackDisabled && ( - - + +