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.

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

[HOLD for payment 2024-08-07] [$250] Web - Profile - Hovering over the zoom tool does not display a tooltip #44029

Closed
1 of 6 tasks
lanitochka17 opened this issue Jun 19, 2024 · 38 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Jun 19, 2024

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.

  1. Navigate to account Settings > Profile
  2. Click on the avatar
  3. Click on upload photo and upload any JPEG image
  4. 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
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6517909_1718758371346.No_tooltip.mp4

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 OwnerCurrent Issue Owner: @ishpaul777
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jun 19, 2024
Copy link

melvin-bot bot commented Jun 19, 2024

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
Copy link
Contributor

Krishna2323 commented Jun 19, 2024

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.

<Tooltip
text={translate('common.zoom')}
shiftVertical={-2}
>
{/* pointerEventsNone is a workaround to make sure the pan gesture works correctly on mobile safari */}
<View style={[styles.sliderKnobTooltipView, styles.pointerEventsNone]} />
</Tooltip>

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
Copy link
Contributor

truph01 commented Jun 20, 2024

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:

    {tooltipIsVisible && (
    <Tooltip
    text={translate('common.zoom')}
    shiftVertical={-2}
    >
    {/* pointerEventsNone is a workaround to make sure the pan gesture works correctly on mobile safari */}
    <View style={[styles.sliderKnobTooltipView, styles.pointerEventsNone]} />
    </Tooltip>
    )}

  • 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:
    {tooltipIsVisible && (
    <Tooltip
    text={translate('common.zoom')}
    shiftVertical={-2}
    >
    {/* pointerEventsNone is a workaround to make sure the pan gesture works correctly on mobile safari */}
    <View style={[styles.sliderKnobTooltipView, styles.pointerEventsNone]} />
    </Tooltip>
    )}

    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>
                    )}

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added the Overdue label Jun 21, 2024
@greg-schroeder greg-schroeder added the External Added to denote the issue can be worked on by a contributor label Jun 25, 2024
@melvin-bot melvin-bot bot changed the title Web - Profile - Hovering over the zoom tool does not display a tooltip [$250] Web - Profile - Hovering over the zoom tool does not display a tooltip Jun 25, 2024
@greg-schroeder
Copy link
Contributor

Sent through for proposal review

Copy link

melvin-bot bot commented Jun 25, 2024

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

@melvin-bot melvin-bot bot added Help Wanted Apply this label when an issue is open to proposals by contributors and removed Overdue labels Jun 25, 2024
Copy link

melvin-bot bot commented Jun 25, 2024

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

@ishpaul777
Copy link
Contributor

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

@ishpaul777
Copy link
Contributor

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
Copy link
Contributor

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

<Tooltip
text={translate('common.zoom')}
shiftVertical={-2}
>
{/* pointerEventsNone is a workaround to make sure the pan gesture works correctly on mobile safari */}
<View style={[styles.sliderKnobTooltipView, styles.pointerEventsNone]} />
</Tooltip>

@dannymcclain
Copy link
Contributor

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
Copy link
Contributor

truph01 commented Jun 27, 2024

@ishpaul777 FYI, we added it in this PR

@ishpaul777
Copy link
Contributor

ishpaul777 commented Jun 28, 2024

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.

🎀 👀 🎀

Copy link

melvin-bot bot commented Jun 28, 2024

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

@truph01
Copy link
Contributor

truph01 commented Jun 28, 2024

@ishpaul777 Can you help check my proposal? This component:

{tooltipIsVisible && (
<Tooltip
text={translate('common.zoom')}
shiftVertical={-2}
>
{/* pointerEventsNone is a workaround to make sure the pan gesture works correctly on mobile safari */}
<View style={[styles.sliderKnobTooltipView, styles.pointerEventsNone]} />
</Tooltip>
)}

is redundant in case the platform does not support hover and we can remove it.

@ishpaul777
Copy link
Contributor

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

if (!hasHoverSupport) {
return children;
}

@truph01
Copy link
Contributor

truph01 commented Jun 28, 2024

  • No. I mean currently, we are using two "zoom" icons. One is:

    <Animated.View style={[styles.sliderKnob, rSliderStyle]}>

    and the other one is:
    <View style={[styles.sliderKnobTooltipView, styles.pointerEventsNone]} />

  • 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
Copy link
Contributor

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
Copy link
Contributor

truph01 commented Jun 28, 2024

@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?

Copy link

melvin-bot bot commented Jul 1, 2024

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

@Krishna2323
Copy link
Contributor

@ishpaul777 PR ready for review ^

@greg-schroeder
Copy link
Contributor

PR still in review

@greg-schroeder
Copy link
Contributor

greg-schroeder commented Jul 16, 2024

This was deployed to prod yesterday but the automation seems not to have picked it up. Hmm. it was deployed to staging twice though, so perhaps we're waiting for a second prod deploy?

@ishpaul777
Copy link
Contributor

This was on prod last week #44788 (comment)

Please hold my payment for few days (End of this month expected ), i'll bump once i am ready to accept payment, Thanks!

@greg-schroeder
Copy link
Contributor

@ishpaul777 I'd really rather pay this on time - is there a reason I should hold off on paying this? It's technically overdue and it's in my queue to be paid.

@greg-schroeder greg-schroeder added Daily KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review Weekly KSv2 labels Jul 24, 2024
@greg-schroeder greg-schroeder changed the title [$250] Web - Profile - Hovering over the zoom tool does not display a tooltip [HOLD for payment 2024-07-24] [$250] Web - Profile - Hovering over the zoom tool does not display a tooltip Jul 24, 2024
@ishpaul777
Copy link
Contributor

i have applied to get a registered with GSTIN in India because upwork is charging me extra 💰 https://support.upwork.com/hc/en-us/articles/360041114713-India-GST-for-freelancers, i have my payments on hold across issues it would be great if we can on this one too.

@greg-schroeder
Copy link
Contributor

All right

@greg-schroeder greg-schroeder changed the title [HOLD for payment 2024-07-24] [$250] Web - Profile - Hovering over the zoom tool does not display a tooltip [HOLD for payment 2024-08-01] [$250] Web - Profile - Hovering over the zoom tool does not display a tooltip Jul 25, 2024
@greg-schroeder
Copy link
Contributor

Is this still the case @ishpaul777 ?

@melvin-bot melvin-bot bot added the Overdue label Jul 30, 2024
Copy link

melvin-bot bot commented Jul 30, 2024

@neil-marcellini, @greg-schroeder, @Krishna2323, @ishpaul777 Eep! 4 days overdue now. Issues have feelings too...

@ishpaul777
Copy link
Contributor

Sadly yes, the process is taking longer than expected, i'll wait for one more week otherwise will take the payment. Thanks for you patience 🙇

@melvin-bot melvin-bot bot removed the Overdue label Jul 30, 2024
@greg-schroeder greg-schroeder added Weekly KSv2 and removed Daily KSv2 labels Jul 31, 2024
@greg-schroeder greg-schroeder changed the title [HOLD for payment 2024-08-01] [$250] Web - Profile - Hovering over the zoom tool does not display a tooltip [HOLD for payment 2024-08-07] [$250] Web - Profile - Hovering over the zoom tool does not display a tooltip Jul 31, 2024
@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 31, 2024
@greg-schroeder
Copy link
Contributor

Hanging tight until @ishpaul777 can get his situation fixed up!

@melvin-bot melvin-bot bot added the Overdue label Aug 5, 2024
@ishpaul777
Copy link
Contributor

Thanks for holding payment @greg-schroeder, feel free to release whenever you get chance. 🙇

@melvin-bot melvin-bot bot removed the Overdue label Aug 5, 2024
@greg-schroeder
Copy link
Contributor

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Archived in project
Development

No branches or pull requests

7 participants