-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
reset tooltip position on scroll #56209
reset tooltip position on scroll #56209
Conversation
3169431
to
e04e6da
Compare
@hoangzinh Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2025-03-06.at.16.41.47.android.movAndroid: mWeb ChromeScreen.Recording.2025-03-06.at.16.38.41.android.chrome.moviOS: NativeScreen.Recording.2025-03-06.at.16.51.11.moviOS: mWeb SafariScreen.Recording.2025-03-06.at.16.44.38.ios.safari.movMacOS: Chrome / SafariScreen.Recording.2025-03-05.at.21.59.59.web.movMacOS: DesktopScreen.Recording.2025-03-05.at.22.04.46.desktop.mov |
@@ -198,6 +198,8 @@ function OptionRowLHN({reportID, isFocused = false, onSelectRow = () => {}, opti | |||
shiftVertical={shouldShowWokspaceChatTooltip ? 0 : variables.composerTooltipShiftVertical} | |||
wrapperStyle={styles.productTrainingTooltipWrapper} | |||
onTooltipPress={onOptionPress} | |||
name={CONST.PRODUCT_TRAINING_TOOLTIP_NAMES.SEARCH_FILTER_BUTTON_TOOLTIP} | |||
shouldHideOnEdge |
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.
Are there any cases that we shouldn't hide on edge?
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.
We hide the tooltip when content is below the header and footer. We have many tooltip which we show on header and footer. So I passed shouldHideOnEdge for the tooltip which should be hide during scrolling and all the other tooltips will be shown as it is.
if (!shouldRender || !name || !shouldHideOnEdge) { | ||
return; | ||
} | ||
setTooltipPosition(false, name); |
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.
In my opinion, it should be clearer if we only put those into setTooltipPosition
method, and define another method (i.e handleTooltipVisibibe
) that will same as current setTooltipPosition
method. Then we can use both of them here. Wdyt?
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.
Currently, this logic based on few flags and name of the tooltip here. If we separate into multiple function we need to add check in both the methods. If I separate handleTooltipVisible() will only have this three lines of code. So I don't think we get more benefits on this. Anyway it is upto you if you want I can refactor, let me know WDYT.
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 thought we always set tooltip position on mount for all educational tooltip. If we separate like this, we don't need to pass default value like (false, name)
in order to bypass those conditions
If I separate handleTooltipVisible() will only have this three lines of code.
Isn't it 9 lines of code here? 👀
if (tooltipName !== name || !genericTooltipStateRef.current || !tooltipElRef.current) { | |
return; | |
} | |
const {hideTooltip, showTooltip, updateTargetBounds} = genericTooltipStateRef.current; | |
if (isScrolling) { | |
hideTooltip(); | |
} else { | |
getTooltipCoordiate(tooltipElRef.current, (bounds) => { |
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.
@hoangzinh not exactly, if we split this function into two function then first three lines(below code) of code needs to be add in both the function so if below condition will be true no code will be executed.
if (tooltipName !== name || !genericTooltipStateRef.current || !tooltipElRef.current) {
return;
}
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.
Also I required const {hideTooltip, showTooltip, updateTargetBounds} = genericTooltipStateRef.current;
line in else block as well so only three lines of code can be moved into another function.
@@ -244,8 +247,9 @@ function LHNOptionsList({style, contentContainerStyles, data, onSelectRow, optio | |||
if (isWebOrDesktop) { | |||
saveScrollIndex(route, Math.floor(e.nativeEvent.contentOffset.y / estimatedItemSize)); | |||
} | |||
triggerScrollEvent(); |
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.
In theory, we only display 1 educational tooltip at a time. Is it easier if we can reuse the existing scrolling event here
App/src/components/InvertedFlatList/index.tsx
Lines 33 to 48 in 0856182
const onScroll = (event: NativeSyntheticEvent<NativeScrollEvent>) => { | |
onScrollProp(event); | |
if (!updateInProgress.current) { | |
updateInProgress.current = true; | |
DeviceEventEmitter.emit(CONST.EVENTS.SCROLLING, true); | |
} | |
}; | |
/** | |
* Emits when the scrolling has ended. | |
*/ | |
const onScrollEnd = () => { | |
DeviceEventEmitter.emit(CONST.EVENTS.SCROLLING, false); | |
updateInProgress.current = false; | |
}; |
And we also introduce additional prop to EducationalTooltip (i.e shouldHideOnScroll). Then we check if it's displayed and shouldHideOnScroll, we will hide/display tooltip. Wdyt?
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.
We need show/hide tooltip in scrolling for SearchPage as well so I have created new hook. And in future we need to add more we can reuse same hook. So as per me current implementation can be reused in other places as well. Let me know if you have any other opinion.
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.
The problem with this approach:
- We're gonna add more places, like InviteMemberListItem, it also use
SelectionList
. - If later, we remove EducationTooltip in those places, let's say this component
LHNOptionsList
, we probably forgot to remove this scroll event emitter as well.
The new hook is a good idea, I like it. It helps reuse. But if we can also have a way to avoid above concerns, that should be great. Any thoughts? Something like put this new hook into SelectionList
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.
@hoangzinh I did not understand what you are trying to say. Can you please provide code snippet about your thoughts?
What I understand is, we need to call triggerScrollEvent() whenever scrolling happened, and also this is not limited to SelectionList right? We can have different types of wrapper which can scroll, so I am not sure if we can make changes in one places and it will be applied to all the places.
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.
In LHNOptionsList
we used FlashList
component while in Search
we use SelectionListWithModal
which internally use SelectionList
so I think it is better idea to use the new hook and trigger the event whenever necessary.
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.
Hmm I see the problem. Basically, what I meant is putting useScrollEventEmitter in SelectionList, so other components using SelectionList, we don't need to initialize useScrollEventEmitter
anymore. Anyways, I'm good with current, but 2 things:
-
Can you add a comment above this line, to explain why do we need this line? It will help to remove/improve later.
const triggerScrollEvent = useScrollEventEmitter({tooltipName: CONST.PRODUCT_TRAINING_TOOLTIP_NAMES.SEARCH_FILTER_BUTTON_TOOLTIP}); -
I'm still thinking that we don't need to inject tooltipName into
useScrollEventEmitter
and simply calluseScrollEventEmitter()
. Reason: In some components, we have 2-3 kinds of EducationalTooltip. For example, inOptionRowLHN
, it has 2 kinds of tooltip
const tooltip = shouldShowGetStartedTooltip ? CONST.PRODUCT_TRAINING_TOOLTIP_NAMES.CONCEIRGE_LHN_GBR : CONST.PRODUCT_TRAINING_TOOLTIP_NAMES.LHN_WORKSPACE_CHAT_TOOLTIP;
If we go with current approach, do we need to setup 2 useScrollEventEmitter?
const triggerScrollEvent1 = useScrollEventEmitter({tooltipName: CONST.PRODUCT_TRAINING_TOOLTIP_NAMES.
});
const triggerScrollEvent2 = useScrollEventEmitter({tooltipName: CONST.PRODUCT_TRAINING_TOOLTIP_NAMES.CONCEIRGE_LHN_GBR});
...
triggerScrollEvent1();
triggerScrollEvent2();
My thoughts here is we can introduce an additional prop shouldHideOnScroll
in BaseEducationalTooltip, then only setup 1 useScrollEventEmitter()
in OptionRowLHN
and pass shouldHideOnScroll={true}
here
App/src/components/LHNOptionsList/OptionRowLHN.tsx
Lines 189 to 201 in fc643d5
<EducationalTooltip | |
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | |
shouldRender={shouldShowProductTrainingTooltip} | |
renderTooltipContent={renderProductTrainingTooltip} | |
anchorAlignment={{ | |
horizontal: shouldShowWokspaceChatTooltip ? CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.LEFT : CONST.MODAL.ANCHOR_ORIGIN_HORIZONTAL.RIGHT, | |
vertical: CONST.MODAL.ANCHOR_ORIGIN_VERTICAL.TOP, | |
}} | |
shiftHorizontal={shouldShowWokspaceChatTooltip ? variables.workspaceLHNtooltipShiftHorizontal : variables.gbrTooltipShiftHorizontal} | |
shiftVertical={shouldShowWokspaceChatTooltip ? 0 : variables.composerTooltipShiftVertical} | |
wrapperStyle={styles.productTrainingTooltipWrapper} | |
onTooltipPress={onOptionPress} | |
> |
wdyt? @mohit6789
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.
Let me try to implement it, and see if this will work or not
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.
Changes pushed
@mohit6789 is it feasible to only hide if the component is nearly or completely hidden? At the moment, it's hidden eventhough component is half-hidden. Screen.Recording.2025-02-27.at.17.40.56.mov |
Sure I will add few more pixels so it will be hide when it reach near the header or footer. |
@hoangzinh do you have any comment on my reply? If not I will fix your last suggestion about hiding the tooltip near the edges? |
Sorry @mohit6789 I just realized that all my feedbacks were pending to submit |
@mohit6789 yes if it's feasible |
Changes pushed |
@hoangzinh I have made review comment changes, please review it again. |
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.
Look good. Just small comment
src/components/Tooltip/types.ts
Outdated
@@ -87,6 +87,9 @@ type EducationalTooltipProps = ChildrenProps & | |||
|
|||
/** Whether the tooltip should hide when navigating */ | |||
shouldHideOnNavigate?: boolean; | |||
|
|||
/** whether tooltip should hide during scrolling */ | |||
shouldHideOnEdge?: boolean; |
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.
shouldHideOnScroll
is clearer for me. What do you think?
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.
Sorry @mohit6789 to make confusing here, can you take a look at this my last feedback here? :thank you:
Oops, it's a wrong click @MonilBhavsar. I still review this PR. I will revert approval |
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.
Revert previous approval
@hoangzinh changes pushed |
return; | ||
} | ||
setTooltipPosition(false); | ||
const scrollingListener = DeviceEventEmitter.addListener(CONST.EVENTS.SCROLLING, ({isScrolling}: ScrollingEventData = {isScrolling: false, tooltipName: ''}) => { |
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.
const scrollingListener = DeviceEventEmitter.addListener(CONST.EVENTS.SCROLLING, ({isScrolling}: ScrollingEventData = {isScrolling: false, tooltipName: ''}) => { | |
const scrollingListener = DeviceEventEmitter.addListener(CONST.EVENTS.SCROLLING, ({isScrolling}: ScrollingEventData = {isScrolling: false}) => { |
It seems we can remove tooltipName 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.
@hoangzinh fixed
@hoangzinh tooltip will be hidden when search box is near the top of header. So if you go a bit up then it will be hide. Previously I have added 10px as a buffer but after your request I have added 30px. Let me know if you want I can change to 20px as buffer. |
@hoangzinh it will be grateful if you tried your preferred value here
|
@mohit6789 Sorry, after playing around, it seems 10 is good to me. |
On Android, if the WS chat is above the last chat, it hides the tooltip. Does it happen in your end? @mohit6789 I did log the Screen.Recording.2025-03-06.at.09.10.04.mov |
@hoangzinh Changes pushed for android issue and make buffer size to 10 |
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.
LGTM
all yours @MonilBhavsar |
src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx
Outdated
Show resolved
Hide resolved
src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx
Outdated
Show resolved
Hide resolved
src/components/Tooltip/EducationalTooltip/measureTooltipCoordinate/index.android.ts
Outdated
Show resolved
Hide resolved
src/components/Tooltip/EducationalTooltip/measureTooltipCoordinate/index.ts
Outdated
Show resolved
Hide resolved
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.
Can we please try to expand tests and QA for all pages that uses tooltips?
@MonilBhavsar We don't have existing test cases for tooltip. So it would be great to create separate task to add test cases for tooltip. You can check my web and desktop recording where most of the education tooltip were tested. |
@MonilBhavsar review changes pushed |
I mean the manual tests. Could you please add the tests for Reports page in the "Tests" and "QA" section in the description |
@MonilBhavsar Updated test section in description |
Co-authored-by: Monil Bhavsar <monilbhavsar25@gmail.com>
@MonilBhavsar updated the comment in BaseEducationalTooltip |
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!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Explanation of Change
Fixed Issues
$#54924
PROPOSAL:#54924 (comment)
Tests
Same as QA
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Scenario 1
Scenario 2
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android.mp4
Android: mWeb Chrome
Android.Web.mp4
iOS: Native
IOS.mp4
iOS: mWeb Safari
IOS.Web.mp4
MacOS: Chrome / Safari
Web.mp4
MacOS: Desktop
Desktop.mp4