-
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
[$250] Submit expenses and chat with approver tooltip appears at the top of the page sometimes #54924
Comments
Triggered auto assignment to @bfitzexpensify ( |
This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989 |
I'm able to reproduce this issue with either new accounts
or set
Screen.Recording.2025-01-08.at.17.42.22.mov |
Please assign me as a C+ for this issue. Thank you. |
🚨 Edited by proposal-police: This proposal was edited at 2025-01-17 14:57:08 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Submit expenses and chat with approver tooltip appears at the top of the page sometimes What is the root cause of that problem?there's no mechanism set to handle scrolling events What changes do you think we should make in order to solve the problem?a more straight forward appraoch, is chekking if element is within view port, this is flexible way to deal with all pages
the above is for web only to include native, it could be something similar to this :
could be polished in PR phase bandicam.2025-01-17.14-48-36-976.mp4What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?UI issue What alternative solutions did you explore? (Optional) |
@hoangzinh feel free to take over, just coming here from here to say that similar thing happen to filter button tooltip when we scroll the expenses on small screens, Make sure we fix it and other tooltips where this potentially happens |
also the expected behaviour is not show tooltip it if element it is pointing to is not in the visible view and ideally we keep the tooltip on it while scrolling pointing to element. Or if that's too hard, have it disappear while scrolling and re-appear once they stop (if the chat is still visible) - https://expensify.slack.com/archives/C07NMDKEFMH/p1736277111466739?thread_ts=1736274954.289009&cid=C07NMDKEFMH |
Done! |
@ishpaul777 @hoangzinh , I guess it's supposed to be educational meaning it leads the user through the app ... I think: bandicam.2025-01-09.16-56-43-609.mp4 |
@M00rish i am sorry but i dont follow what you mean by this tooltip is only shown to active (default) workspace chat of user, what do you propose? |
🚨 Edited by proposal-police: This proposal was edited at 2025-01-12 12:42:58 UTC. 🚨 Edited by proposal-police: This proposal was edited at 2025-01-12 12:42:58 UTC. Edited by proposal-police: This proposal was edited at 2025-01-12 12:42:58 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Submit expenses and chat with approver tooltip appears at the top of the page sometimes What is the root cause of that problem?We are not updating the position of the tooltip when the layout of the content changes(Because the
What changes do you think we should make in order to solve the problem?To address this issue, we can use const elementRef = useRef();
const [isVisibleElement, setIsVisibleElement] = useState(false);
const getBounds = (bounds: DOMRect): LayoutRectangle => {
const targetElement = elementRef.current?._childNode;
const elementAtPoint = document.elementFromPoint(bounds.x, bounds.y + bounds.height / 2); //Consider increase x by + padding
if (elementAtPoint && 'contains' in elementAtPoint && targetElement && 'contains' in targetElement) {
if (shouldCheckBottomVisible) {// This flag is true in LHN
const viewportHeight = window.innerHeight; // The height of the visible viewport
const isBottomVisible = bounds.bottom + bounds.height <= viewportHeight; //Consider decrease viewportHeight by - padding
const isInViewport = isBottomVisible;
if (!isInViewport) {
setIsVisibleElement(false);
return bounds
}
}
const isElementVisible =
elementAtPoint instanceof HTMLElement &&
(targetElement?.contains(elementAtPoint) || elementAtPoint?.contains(targetElement)); // We can update condition here, if we need other expected
setIsVisibleElement(isElementVisible)
}
return bounds;
};
...
<BoundsObserver
enabled={shouldRender}
onBoundsChange={(bounds) => {
updateTargetBounds(getBounds(bounds));
}}
ref={elementRef}
>
{React.cloneElement(children as React.ReactElement, {
onLayout: (e: LayoutChangeEventWithTarget) => {
if (!shouldMeasure) {
setShouldMeasure(true);
}
// e.target is specific to native, use e.nativeEvent.target on web instead
const target = e.target || e.nativeEvent.target;
show.current = () => measureTooltipCoordinate(target, updateTargetBounds, showTooltip);
},
})}
</BoundsObserver> We need
shouldRender={shouldRender && isVisibleElement} For the detailed code, we can discuss it later in PR. POC
What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?No, UI bug What alternative solutions did you explore? (Optional)Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job. |
@hoangzinh, @bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Job added to Upwork: https://www.upwork.com/jobs/~021878933333602082321 |
Current assignee @hoangzinh is eligible for the External assigner, not assigning anyone new. |
Yes, I tried to not use GenericTooltip here but use native wrapper <View style={styles.pRelative}>
<View style={[styles.pAbsolute, {top: 0, left: 100}]}>Tooltip</View>
{children}
</View> |
@hoangzinh May be we can do this way (Still I am not sure, if this will work in Mobile or not), but this required major refactor that is what I am trying to say. So my first question is do we really want to change the architecture of the EducationalTooltip? @MonilBhavsar We already spend quite huge amount of time to check few approaches. Now we are moving into completely different approach. |
Hi @ishpaul777 how important is it for the migration project that you mentioned here? I mean how urgent is it? |
@flaviadefaria would be the best person to judge the urgency but it seems like we are planning to add more educational tooltips so a general solution to this is pretty important |
Thanks @ishpaul777. Hmm, It seems we still have enough time to look for a solution that makes the product training tooltips to follow the element that it is pointing to when scrolling. What do you think @MonilBhavsar? Or are you good with current suggestion from @mohit6789? |
I am fine with the #54924 (comment) suggestion But it should only impact the tooltips where target is moving not the ones that are static (e.g. FAB tooltips). |
I am concerned about the performance issues if we decide to display the tooltip while scrolling if it tries to re render often. |
Just to clarify I was talking about this #54924 (comment) suggestion. |
@MonilBhavsar Yep, with the current Educational tooltip implementation, it's hard (and the tooltip will blink or laggy) if we want to display the tooltip while scrolling. |
Yeah, that makes sense. I think it couldn't hurt to try it, but I am cool with the solution that hides it while scrolling and then makes it reappear - that feels pretty natural to me. |
Yes, asked gpt about this and it also suggests hidding while scrolling. So I think we could go with @mohit6789's proposal |
@mohit6789 feel free to open a PR |
@mohit6789 @hoangzinh Do test your solution againstt all Tooltips in the App. Thank you. |
@hoangzinh, @mohit6789, @MonilBhavsar, @bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Working on this |
Cool. Let me know when it's ready to review |
@hoangzinh Huh... This is 4 days overdue. Who can take care of this? |
@mohit6789 Do you have any timeframe for when the PR will be ready for review? |
IOS and android builds are not working, I am working to fix this. I will create PR ASAP |
@mohit6789 We will have to reassign this issue if PR is not created in a few hours. It's been 2 weeks. cc: @hoangzinh We are dependent on this PR. |
@parasharrajat I can create PR by tomorrow. Changes are almost done. Please allow one more day. |
@hoangzinh @parasharrajat PR is ready for review. |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: v9.0.81-6
Reproducible in staging?: yes
Reproducible in production?: yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @flaviadefaria
Slack conversation (hyperlinked to channel name): migrate
Action Performed:
Expected Result:
Should appear with in the report active window
Actual Result:
sometimes displays at the top of the page. Also when I scroll down the chats on the LHP it stays static rather than following the workspace that it is pointing to.
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
2025-01-06_21-26-00.1.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @Issue Owner
Current Issue Owner: @hoangzinhThe text was updated successfully, but these errors were encountered: