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.

Sign up for GitHub

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

Fix tooltip for QAB doesn't show for new user #52446

Merged
merged 9 commits into from
Nov 26, 2024
Merged

Fix tooltip for QAB doesn't show for new user #52446

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1051bda
fix tooltip for QAB doesn't show for new user
bernhardoj Nov 13, 2024
da87273
lint
bernhardoj Nov 13, 2024
3b05518
lint
bernhardoj Nov 13, 2024
98bc0ea
Merge branch 'main' into fix/51987-missing-tooltip-for-qab
bernhardoj Nov 21, 2024
c3efa31
add comment
bernhardoj Nov 21, 2024
7538b46
use set
bernhardoj Nov 21, 2024
2fc61f7
typo
bernhardoj Nov 21, 2024
e6ed429
add more comment
bernhardoj Nov 21, 2024
8aec50b
add new line
bernhardoj Nov 26, 2024
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
47 changes: 11 additions & 36 deletions src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@ import React, {memo, useCallback, useEffect, useRef, useState} from 'react';
import type {LayoutRectangle, NativeSyntheticEvent} from 'react-native';
import GenericTooltip from '@components/Tooltip/GenericTooltip';
import type {EducationalTooltipProps} from '@components/Tooltip/types';
import onyxSubscribe from '@libs/onyxSubscribe';
import ONYXKEYS from '@src/ONYXKEYS';
import type {Modal} from '@src/types/onyx';
import measureTooltipCoordinate from './measureTooltipCoordinate';
import * as TooltipManager from './TooltipManager';

type LayoutChangeEventWithTarget = NativeSyntheticEvent<{layout: LayoutRectangle; target: HTMLElement}>;

Expand All @@ -18,30 +16,8 @@ function BaseEducationalTooltip({children, onHideTooltip, shouldRender = false,

const [shouldMeasure, setShouldMeasure] = useState(false);
const show = useRef<() => void>();
const [modal, setModal] = useState<Modal>({
willAlertModalBecomeVisible: false,
isVisible: false,
});

const shouldShow = !modal?.willAlertModalBecomeVisible && !modal?.isVisible && shouldRender;

useEffect(() => {
if (!shouldRender) {
return;
}
const unsubscribeOnyxModal = onyxSubscribe({
key: ONYXKEYS.MODAL,
callback: (modalArg) => {
if (modalArg === undefined) {
return;
}
setModal(modalArg);
},
});
return () => {
unsubscribeOnyxModal();
};
}, [shouldRender]);
const removeActiveTooltipRef = useRef(() => {});
const removePendingToltipRef = useRef(() => {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const removePendingToltipRef = useRef(() => {});
const removePendingTooltipRef = useRef(() => {});


const didShow = useRef(false);

Expand All @@ -51,6 +27,7 @@ function BaseEducationalTooltip({children, onHideTooltip, shouldRender = false,
}
hideTooltipRef.current?.();
onHideTooltip?.();
removeActiveTooltipRef.current();
}, [onHideTooltip]);

useEffect(
Expand All @@ -70,34 +47,32 @@ function BaseEducationalTooltip({children, onHideTooltip, shouldRender = false,
return;
}

// If the modal is open, hide the tooltip immediately and clear the timeout
if (!shouldShow) {
closeTooltip();
return;
}

// Automatically hide tooltip after 5 seconds if shouldAutoDismiss is true
const timerID = setTimeout(() => {
closeTooltip();
}, 5000);
return () => {
clearTimeout(timerID);
};
}, [shouldAutoDismiss, shouldShow, closeTooltip]);
}, [shouldAutoDismiss, closeTooltip]);

useEffect(() => {
if (!shouldMeasure || !shouldShow || didShow.current) {
if (!shouldMeasure || !shouldRender || didShow.current) {
return;
}
// When tooltip is used inside an animated view (e.g. popover), we need to wait for the animation to finish before measuring content.
const timerID = setTimeout(() => {
removePendingToltipRef.current();
show.current?.();
didShow.current = true;
removeActiveTooltipRef.current = TooltipManager.addActiveTooltip(closeTooltip);
}, 500);
removePendingToltipRef.current = TooltipManager.addPendingTooltip(timerID);
return () => {
removePendingToltipRef.current();
clearTimeout(timerID);
};
}, [shouldMeasure, shouldShow]);
}, [shouldMeasure, shouldRender, closeTooltip]);

useEffect(
() => closeTooltip,
Expand Down
29 changes: 29 additions & 0 deletions src/components/Tooltip/EducationalTooltip/TooltipManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
const pendingTooltip: NodeJS.Timeout[] = [];
const tooltipCloseCallback: Array<() => void> = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const pendingTooltip: NodeJS.Timeout[] = [];
const tooltipCloseCallback: Array<() => void> = [];
const pendingTooltips = new Set<NodeJS.Timeout>();
const activeTooltips = new Set<() => void>();

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const pendingTooltip: NodeJS.Timeout[] = [];
const tooltipCloseCallback: Array<() => void> = [];
// We store the timeouts for each pending tooltip here. We're using the timeout because when a tooltip is used inside an animated view (e.g., popover), we need to wait for the animation to finish before measuring content.
const pendingTooltipsTimouts: NodeJS.Timeout[] = [];
// We store the callback for closing a tooltip here.
const closeActiveTooltipCallbacks: Array<() => void> = [];


function addPendingTooltip(timeout: NodeJS.Timeout) {
pendingTooltip.push(timeout);
return () => {
const index = pendingTooltip.indexOf(timeout);
pendingTooltip.splice(index, 1);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pendingTooltip.push(timeout);
return () => {
const index = pendingTooltip.indexOf(timeout);
pendingTooltip.splice(index, 1);
};
pendingTooltips.add(timeout);
return () => {
pendingTooltips.delete(timeout);
};

}

function addActiveTooltip(closeCallback: () => void) {
tooltipCloseCallback.push(closeCallback);
return () => {
const index = tooltipCloseCallback.indexOf(closeCallback);
tooltipCloseCallback.splice(index, 1);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tooltipCloseCallback.push(closeCallback);
return () => {
const index = tooltipCloseCallback.indexOf(closeCallback);
tooltipCloseCallback.splice(index, 1);
};
activeTooltips.add(closeCallback);
return () => {
activeTooltips.delete(closeCallback);
};

}

function cancelPendingAndActiveTooltips() {
while (pendingTooltip.length > 0) {
clearTimeout(pendingTooltip.pop());
}
while (tooltipCloseCallback.length > 0) {
tooltipCloseCallback.pop()?.();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
while (pendingTooltip.length > 0) {
clearTimeout(pendingTooltip.pop());
}
while (tooltipCloseCallback.length > 0) {
tooltipCloseCallback.pop()?.();
}
pendingTooltips.forEach((timeout) => clearTimeout(timeout));
pendingTooltips.clear();
activeTooltips.forEach((closeCallback) => closeCallback());
activeTooltips.clear();

}

export {addPendingTooltip, addActiveTooltip, cancelPendingAndActiveTooltips};
2 changes: 2 additions & 0 deletions src/libs/Navigation/AppNavigator/AuthScreens.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {SearchContextProvider} from '@components/Search/SearchContext';
import {useSearchRouterContext} from '@components/Search/SearchRouter/SearchRouterContext';
import SearchRouterModal from '@components/Search/SearchRouter/SearchRouterModal';
import TestToolsModal from '@components/TestToolsModal';
import * as TooltipManager from '@components/Tooltip/EducationalTooltip/TooltipManager';
import useActiveWorkspace from '@hooks/useActiveWorkspace';
import useOnboardingFlowRouter from '@hooks/useOnboardingFlow';
import usePermissions from '@hooks/usePermissions';
Expand Down Expand Up @@ -202,6 +203,7 @@ const RootStack = createCustomStackNavigator<AuthScreensParamList>();

const modalScreenListeners = {
focus: () => {
TooltipManager.cancelPendingAndActiveTooltips();
Modal.setModalVisibility(true);
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And because the RHP modal uses setModalVisbility, we need to manually cancel it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a comment in the code for this?

blur: () => {
Expand Down
4 changes: 4 additions & 0 deletions src/libs/actions/Modal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Onyx from 'react-native-onyx';
import * as TooltipManager from '@components/Tooltip/EducationalTooltip/TooltipManager';
import ONYXKEYS from '@src/ONYXKEYS';

const closeModals: Array<(isNavigating?: boolean) => void> = [];
Expand Down Expand Up @@ -86,6 +87,9 @@ function setDisableDismissOnEscape(disableDismissOnEscape: boolean) {
* isPopover indicates that the next open modal is popover or bottom docked
*/
function willAlertModalBecomeVisible(isVisible: boolean, isPopover = false) {
if (isVisible) {
TooltipManager.cancelPendingAndActiveTooltips();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only cancel it in willAlertModalBecomeVisible and not in setModalVisibility because setModalVisibility is called when the modal is shown, not when the modal will show.

App/src/components/Modal/BaseModal.tsx

Lines 102 to 109 in 7256ad6

useEffect(() => {
isVisibleRef.current = isVisible;
let removeOnCloseListener: () => void;
if (isVisible) {
Modal.willAlertModalBecomeVisible(true, type === CONST.MODAL.MODAL_TYPE.POPOVER || type === CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED);
// To handle closing any modal already visible when this modal is mounted, i.e. PopoverReportActionContextMenu
removeOnCloseListener = Modal.setCloseModal(onClose);
}

App/src/components/Modal/BaseModal.tsx

Lines 131 to 136 in 7256ad6

const handleShowModal = () => {
if (shouldSetModalVisibility) {
Modal.setModalVisibility(true);
}
onModalShow();
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a comment in the code for this?

Onyx.merge(ONYXKEYS.MODAL, {willAlertModalBecomeVisible: isVisible, isPopover});
}

Expand Down
Loading