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 2023-06-23] [HOLD for payment 2023-06-21] Web - Chat - Cursor on staging link is not pointer #20669

Closed
1 of 6 tasks
kbecciv opened this issue Jun 13, 2023 · 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 Engineering Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Jun 13, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Open the app
  2. Open any report
  3. Copy current URL and send in report
  4. Hover on link to observe that cursor is not pointer

Expected Result:

App should display pointer cursor on links

Actual Result:

App displays normal cursor on staging expensify links

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.27.2

Reproducible in staging?: Yes

Reproducible in production?: no

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

Notes/Photos/Videos: Any additional supporting documentation

cursor.on.staging.is.not.pointer.mp4
Recording.3087.mp4

Expensify/Expensify Issue URL:

Issue reported by: @adeel0202

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686648377836269

View all open jobs on GitHub

@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Jun 13, 2023
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@melvin-bot
Copy link

melvin-bot bot commented Jun 13, 2023

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

@youssef-lr
Copy link
Contributor

On it.

@eh2077
Copy link
Contributor

eh2077 commented Jun 13, 2023

It's possibly related to #19717 as their fix changed styles of link.

@youssef-lr
Copy link
Contributor

youssef-lr commented Jun 13, 2023

thanks @eh2077! That's indeed the offending PR, and specifically this line, as soon as I remove styles.cursorDefault from the styles array the link works. However, what's weird is this is only affecting links like this: https://staging.new.expensify.com/{+text here}, why isn't this affecting all links? For example: this works fine https://staging.new.expensify.com/

@youssef-lr
Copy link
Contributor

youssef-lr commented Jun 13, 2023

Actually this is the offending PR #19545.

@grgia
Copy link
Contributor

grgia commented Jun 13, 2023

@youssef-lr all these PRs seem to be linked to #19717

I think we should consider reverting

@Nikhil-Vats
Copy link
Contributor

This is caused by my PR but reverting it is not the solution because we were adding the pointer cursor not to the link but to its parent which showed the pointer at places around the link -

Screen.Recording.2023-06-13.at.9.46.29.PM.mov

The root cause for this issue is that for some reasons links like https://staging.new.expensify.com/random_text_here are rendered in div rather than a which is wrong.

Screen.Recording.2023-06-13.at.9.48.01.PM.mov

So, we need to figure out why links are rendered in div and change it to a to fix it the right way.

@youssef-lr
Copy link
Contributor

Ahh gothcu. Do you think you might be able to find a fix? We can always revert than implement the new solution in a follow up PR as this is a deploy blocker. If we can't find the fix quickly then we'll have to revert.

@parasharrajat
Copy link
Member

I am sure it will take time to find a good solution as that PR was trying to solve two issues together and finding a balanced solution for both is hard. The fastest option will be to revert the root cause PR and rework.

@youssef-lr
Copy link
Contributor

Sounds good to me @parasharrajat.

@youssef-lr
Copy link
Contributor

youssef-lr commented Jun 13, 2023

@parasharrajat another clue, why do links to https://new.expensify.com/r/801524823110380 e.g. not staging, work? They're internal as well. Do you think we're doing something differently when handling staging links?

Scratch that, I think it might be because we're testing production links from staging environment. If that code hit production, it would fail for production links on production. Correct?

@youssef-lr youssef-lr removed the DeployBlockerCash This issue or pull request should block deployment label Jun 13, 2023
@Nikhil-Vats
Copy link
Contributor

Nikhil-Vats commented Jun 13, 2023

@parasharrajat @youssef-lr I found a solution basically for internal links since they are rendered as div we can explicitly set cursor pointer here. For normal links it is added by default so the only behaviour we are changing is for internal links by adding pointer cursor -

<Tooltip text={props.href}>
<Text
ref={(el) => (linkRef = el)}
style={StyleSheet.flatten([props.style, defaultTextStyle])}

This would solve all the related issues without regression. I tested on local all issues that were related to my issue like #18658, #17488, #16526, etc.

@parasharrajat
Copy link
Member

This issue is resolved because the revert is merged. @Nikhil-Vats you can continue the discussion on the original issue so that C+ can help with the discussion.

@Nikhil-Vats
Copy link
Contributor

Sure @parasharrajat, but since you are aware about the internal links special handling and all the related issues. can you provide your suggestions on the solution above?

@parasharrajat
Copy link
Member

I can help with it but currently busy so can't do it immediately.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Hourly KSv2 labels Jun 14, 2023
@melvin-bot melvin-bot bot added the Daily KSv2 label Jun 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jun 28, 2023

@youssef-lr Whoops! This issue is 2 days overdue. Let's get this updated quick!

@parasharrajat
Copy link
Member

This can be just closed. There is no payment pending here. It was fixed when we reverted the PR.

@adeel0202
Copy link
Contributor

There is no payment pending here.

I'm eligible for reporting bonus I guess 😅

@parasharrajat
Copy link
Member

Surely, missed that.

@melvin-bot
Copy link

melvin-bot bot commented Jun 30, 2023

@youssef-lr Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot
Copy link

melvin-bot bot commented Jul 4, 2023

@youssef-lr Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

@melvin-bot
Copy link

melvin-bot bot commented Jul 6, 2023

@youssef-lr 10 days overdue. Is anyone even seeing these? Hello?

@melvin-bot melvin-bot bot removed the Daily KSv2 label Jul 11, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jul 11, 2023

This issue has not been updated in over 14 days. @youssef-lr eroding to Weekly issue.

@melvin-bot melvin-bot bot added the Weekly KSv2 label Jul 11, 2023
@parasharrajat
Copy link
Member

@youssef-lr Bump! This is awaiting action from you before we can close this. Thanks.

@youssef-lr
Copy link
Contributor

@parasharrajat I can go ahead and close this?

@parasharrajat
Copy link
Member

If you have already paid out the reporting bonus, feel free to close this. #20669 (comment)

@adeel0202
Copy link
Contributor

I'm not paid for reporting yet. @youssef-lr

@adeel0202
Copy link
Contributor

@youssef-lr, bump

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Aug 21, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 21, 2023

This issue has not been updated in over 15 days. @youssef-lr eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@sophiepintoraetz sophiepintoraetz added the Bug Something is broken. Auto assigns a BugZero manager. label Aug 28, 2023
@melvin-bot
Copy link

melvin-bot bot commented Aug 28, 2023

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Monthly KSv2 labels Aug 28, 2023
@sophiepintoraetz
Copy link
Contributor

FYI - @sakluger - all that's needed here is the reporting payment for @adeel0202 ($250)

@Expensify Expensify deleted a comment from melvin-bot bot Aug 29, 2023
@sakluger
Copy link
Contributor

Just sent you an offer through upwork @adeel0202!

https://www.upwork.com/jobs/~0117619b8e4dbdc25f

@adeel0202
Copy link
Contributor

Thanks @sakluger. I have accepted the offer.

@sakluger
Copy link
Contributor

All paid @adeel0202! Sorry again for the wait, and thanks for following up with us.

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 Engineering Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

10 participants