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

Long click on hyper link at bottom it display some background shadow - reported by @dharmik #7796

Closed
mvtglobally opened this issue Feb 17, 2022 · 19 comments
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Monthly KSv2

Comments

@mvtglobally
Copy link

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


Action Performed:

  1. Open app
  2. Long press on any link in the footer

Expected Result:

Long press that background shadow must not be there

Actual Result:

Long press that background shadow is there

Workaround:

unknown

Platform:

Where is this issue occurring?

  • iOS
  • Android
  • Mobile Web

Version Number: 1.1.38-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Screen.Recording.2022-02-04.at.12.05.05.AM.mov

Expensify/Expensify Issue URL:
Issue reported by: @dharmik
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1643913495412989

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Feb 17, 2022
@MelvinBot
Copy link

Triggered auto assignment to @JmillsExpensify (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Feb 17, 2022
@JmillsExpensify

This comment was marked as off-topic.

@parasharrajat
Copy link
Member

@JmillsExpensify I think a lot of process steps got skipped. This is not triaged yet.

@JmillsExpensify
Copy link

JmillsExpensify commented Feb 17, 2022

Oh yeah I see what you mean. I'm so used to the External label for this repo but I was assigned for Triage. I'll hide the Upwork job for now.

@JmillsExpensify JmillsExpensify added Engineering Improvement Item broken or needs improvement. labels Feb 17, 2022
@MelvinBot
Copy link

Triggered auto assignment to @nkuoch (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@dharmik
Copy link
Contributor

dharmik commented Feb 17, 2022

@JmillsExpensify

Proposal:
Just one line we have to add and it will resolved background shadow.

FileName: TextLink.js
Line to add:suppressHighlighting={true}

  <Text
        style={[styles.link, ...additionalStyles]}
        accessibilityRole="link"
        href={props.href}
        onPress={(e) => {
            e.preventDefault();
            if (props.onPress) {
                props.onPress();
                return;
            }
            Linking.openURL(props.href);
        }}
        suppressHighlighting={true}
    >
        {props.children}
    </Text>

@nkuoch
Copy link
Contributor

nkuoch commented Feb 17, 2022

Double checking with @AndrewGable if this is something we want to fix?

@talhatanveer333
Copy link

Proposal:
We just have to wrap a TouchableWithoutFeedback around it. In this way, we can add another layer to it for consistent performance.

Terms of Service

@AndrewGable
Copy link
Contributor

I agree I am not sure I see the bug here, looks to be working as expected?

@JmillsExpensify
Copy link

Ah yeah I don't think this is critical, but there is no feedback on hover over for the link/URL. That's how I interpreted this issue, that it's inconsistent with the behavior for links. I'm not super passionate either way though.

@AndrewGable
Copy link
Contributor

Got it - So we should have no shadow on long press on mobile because there is no hover feedback for web? That makes sense if so!

@JmillsExpensify
Copy link

Let's see what @shawnborton thinks before moving forward.

@shawnborton
Copy link
Contributor

Just to confirm, this only happens when you long-tap? Just a simple tap doesn't launch the bg color right? I kind of think this is fine as it is. I suppose we could even actually keep the behavior but specify the background color we want.

@JmillsExpensify
Copy link

This is super minor stuff, to be clear. That said, we also approach the links differently on OldDot versus NewDot. On desktop, OldDot changes color to provide feedback, whereas NewDot is static (no feedback). Then on mobile, long tapping the homepage link on OldDot uses the existing background, whereas on NewDot we use a grey background. So really whatever we do, we should probably make it consistent.

@JmillsExpensify
Copy link

Another matter entirely is whether this is important enough to do anything. I originally thought this was good for consistency, but I also could see the argument that this is so minor and we have more important pursuits that we can also choose to close the issue.

@JmillsExpensify JmillsExpensify added Weekly KSv2 and removed Daily KSv2 labels Feb 18, 2022
@shawnborton
Copy link
Contributor

I like the idea of treating long-press on mobile/tablets the same as :hover on desktop, and then aligning the desired effect between both behaviors.

@nkuoch nkuoch removed their assignment Feb 22, 2022
@JmillsExpensify
Copy link

Going to put a monthly on this and came back to it. It's not a priority for now.

@MelvinBot MelvinBot removed the Overdue label Mar 2, 2022
@JmillsExpensify JmillsExpensify removed the Weekly KSv2 label Mar 2, 2022
@JmillsExpensify
Copy link

Focused on another stuff right now.

@JmillsExpensify
Copy link

I'm just going to close this and we can re-open down the line when details like this are more important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Monthly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants