[$250] Web - Profile - Hovering over the zoom tool does not display a tooltip

lanitochka17 commented 2 weeks ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.4.85-3 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/a Issue reported by: Applause - Internal Team

Action Performed:

Prerequisite: The user is logged in to the website.

Navigate to account Settings > Profile
Click on the avatar
Click on upload photo and upload any JPEG image
Hover over the zoom tool

Expected Result:

A tooltip is displayed indicating the zoom tool

Actual Result:

A tooltip is not displayed when the user hovers over the zoom tool

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/1407f36a-b9f7-4682-b532-d8623b1c887b

No_tooltip

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~016a8bfe25d7d10e4d
Upwork Job ID: 1805408259060604733
Last Price Increase: 2024-06-25
Automatic offers:

ishpaul777 | Reviewer | 102948392
Krishna2323 | Contributor | 102948394

Issue Owner

Current Issue Owner: @ishpaul777

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Krishna2323 commented 2 weeks ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Web - Profile - Hovering over the zoom tool does not display a tooltip

What is the root cause of that problem?

The tooltip doesn't work because of styles.pointerEventsNone. https://github.com/Expensify/App/blob/e0333147b437fc3639d24d7de1e6ca9bb0fdc63c/src/components/AvatarCropModal/Slider.tsx#L60-L66

What changes do you think we should make in order to solve the problem?

Since the styles.pointerEventsNone style is applied for mobile safari and hover doesn't work on touch enabled devices, we can add a check to only add that style for mobile safari.

<View style={[styles.sliderKnobTooltipView, Browser.isMobileSafari() && styles.pointerEventsNone]} />

What alternative solutions did you explore? (Optional)

truph01 commented 1 week ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

A tooltip is not displayed when the user hovers over the zoom tool

What is the root cause of that problem?

In this PR, we add a logic to display the Slider tooltip: https://github.com/Expensify/App/blob/75614394a2fb1e8114e35bb2d8d33bdd0c565946/src/components/AvatarCropModal/Slider.tsx#L59-L67
It will make sure when slider is panning, the tooltip does not display.
Then in this PR, we need to add the styles.pointerEventsNone to make sure the pan gesture works correctly on mobile safari.
And because of styles.pointerEventsNone, the tooltip is not displayed in desktop when hovering over the slider.

What changes do you think we should make in order to solve the problem?
Because the tooltip only appears in the device that has hover support, so we can remove the: https://github.com/Expensify/App/blob/75614394a2fb1e8114e35bb2d8d33bdd0c565946/src/components/AvatarCropModal/Slider.tsx#L59-L67 if DeviceCapabilities.hasHoverSupport() is false then remove styles.pointerEventsNone styles. Below is detail:
```
{tooltipIsVisible && hasHoverSupport && (
                    <Tooltip
                        text={translate('common.zoom')}
                        shiftVertical={-2}
                    >
                        <View style={[styles.sliderKnobTooltipView]} />
                    </Tooltip>
                )}
```
Then, we need to remove the unnecessary call like: https://github.com/Expensify/App/blob/75614394a2fb1e8114e35bb2d8d33bdd0c565946/src/components/AvatarCropModal/Slider.tsx#L39 and https://github.com/Expensify/App/blob/75614394a2fb1e8114e35bb2d8d33bdd0c565946/src/components/AvatarCropModal/Slider.tsx#L46 if hasHoverSupport is false.

What alternative solutions did you explore? (Optional)

greg-schroeder commented 1 week ago

Sent through for proposal review

melvin-bot[bot] commented 1 week ago

Job added to Upwork: https://www.upwork.com/jobs/~016a8bfe25d7d10e4d

melvin-bot[bot] commented 1 week ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ishpaul777 (External)

ishpaul777 commented 1 week ago

i'll review proposal tomorrow, i have limited availability today

ishpaul777 commented 6 days ago

Do we even need a tooltip for zoom slider anchor i am unsure if we should consider this as bug.

cc @Expensify/design Any thoughts?

Krishna2323 commented 6 days ago

@ishpaul777, we always had the tooltip, it isn't displaying because of styles.pointerEventsNone.

https://github.com/Expensify/App/blob/e0333147b437fc3639d24d7de1e6ca9bb0fdc63c/src/components/AvatarCropModal/Slider.tsx#L60-L66

dannymcclain commented 5 days ago

I guess if we've historically had it, we should probably bring it back. That said, this doesn't feel super critical to me.

truph01 commented 5 days ago

@ishpaul777 FYI, we added it in this PR

ishpaul777 commented 5 days ago

I guess if we've historically had it, we should probably bring it back. That said, this doesn't feel super critical to me.

Thanks for clarifying 👍

We can go with @Krishna2323's proposal, as its the simplest one.

🎀 👀 🎀

melvin-bot[bot] commented 5 days ago

Triggered auto assignment to @neil-marcellini, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

truph01 commented 5 days ago

@ishpaul777 Can you help check my proposal? This component: https://github.com/Expensify/App/blob/75614394a2fb1e8114e35bb2d8d33bdd0c565946/src/components/AvatarCropModal/Slider.tsx#L59-L67 is redundant in case the platform does not support hover and we can remove it.

ishpaul777 commented 5 days ago

Its not necessary, incase hover is not supported we'd have it handled here

https://github.com/Expensify/App/blob/1d607acab9c47f2e0da4e8c85de5d90e2d47c11b/src/components/Tooltip/BaseTooltip/index.tsx#L91-L93

truph01 commented 5 days ago

No. I mean currently, we are using two "zoom" icons. One is: https://github.com/Expensify/App/blob/75614394a2fb1e8114e35bb2d8d33bdd0c565946/src/components/AvatarCropModal/Slider.tsx#L58 and the other one is: https://github.com/Expensify/App/blob/75614394a2fb1e8114e35bb2d8d33bdd0c565946/src/components/AvatarCropModal/Slider.tsx#L65
The 2nd one is used to handle the case: when the user hovers over the zoom, we want to display the tooltip, but when user drags the zoom icon, we do not want to display the tooltip.
So in case the platform does not support hover, we can remove the 2nd zoom button.

ishpaul777 commented 5 days ago

That would just mean we are adding the platform specific conditional hasHoverSupport everywhere , that is just not worth it to remove just one extra IMO lets keep it as it is for the sake of simplicity. That being said i'll let @neil-marcellini make final call on assignment

truph01 commented 5 days ago

@ishpaul777 Thanks for your feedback. Just want to wrap up my solution for @neil-marcellini:

Currently, our Zoom button is displayed as:

<View style={styles.circle}>
<Tooltip>
<View style={styles.circle}>
<View />
</Tooltip>
</View>

So why isn't it just:

<View style={styles.circle} />

in case the platform does not have hover effect?

melvin-bot[bot] commented 2 days ago

@neil-marcellini, @greg-schroeder, @ishpaul777 Whoops! This issue is 2 days overdue. Let's get this updated quick!

neil-marcellini commented 2 days ago

We can go with @Krishna2323's proposal, as its the simplest one.

🎀 👀 🎀

Ok, I'm good with this since it's simple and this bug isn't very important.

melvin-bot[bot] commented 2 days ago

📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 2 days ago

📣 @Krishna2323 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

melvin-bot[bot] commented 2 hours ago

@neil-marcellini @greg-schroeder @Krishna2323 @ishpaul777 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

ishpaul777 commented 2 hours ago

@Krishna2323 whenever you get chance, Please update when can we get a PR up for review. Thank you!

Expensify / App