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

[PAID][$500] NewDot App scroll is 2-3x faster than other apps #10654

Closed
mvtglobally opened this issue Aug 29, 2022 · 59 comments
Closed

[PAID][$500] NewDot App scroll is 2-3x faster than other apps #10654

mvtglobally opened this issue Aug 29, 2022 · 59 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@mvtglobally
Copy link

mvtglobally commented Aug 29, 2022

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. Navigate to any chat with a lot of messages
  2. Scroll through the tread quickly
  3. Leave your finger on the trackpad and move it a little

Expected Result:

Match the scroll behaviour with native apps, having a different speed will influence the users as they will notice it differs and there is no reason for this

Actual Result:

One trackpad of scrolling in Slack seems to scroll about 2/3 of a screen -- which feels "right". But one trackpad of scrolling in NewDot scrolls like 3 screens, which feels like way too much.

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.91-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Recording.526.mp4

Expensify/Expensify Issue URL:
Issue reported by: @quinthar
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1661767055711919?thread_ts=1661331711.805659&cid=C01GTK53T8Q

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Aug 29, 2022

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

@luacmartins
Copy link
Contributor

I couldn't reproduce this. I'll mark it as external to get more eyes on it though.

@luacmartins luacmartins added the External Added to denote the issue can be worked on by a contributor label Aug 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 30, 2022

Triggered auto assignment to @laurenreidexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@laurenreidexpensify laurenreidexpensify changed the title The speed at which user scroll everywhere else is not the same speed used by new dot The speed at which users scroll in other products is not the same speed used by new dot Aug 30, 2022
@laurenreidexpensify
Copy link
Contributor

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Aug 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 30, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 30, 2022

Current assignee @luacmartins is eligible for the Exported assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title The speed at which users scroll in other products is not the same speed used by new dot [$250] The speed at which users scroll in other products is not the same speed used by new dot Aug 30, 2022
@parasharrajat
Copy link
Member

what are we expecting for the base unit for the scroll?

I think we can simply set the Scroll distance to a fixed set of lines. For e.g.

this.list.getScrollableNode().scrollTop -= e.deltaY;

  1. Get the line Height and use this for calculating the scroll distance.
  		const computedStyle = window.getComputedStyle(this.list.getScrollableNode());
        const listLineHeight = parseInt(computedStyle.lineHeight, 10) || 20;
            
        let scale = e.deltaY;
        if (e.deltaMode === e.DOM_DELTA_PIXEL) {
            scale = listLineHeight * 3
        }
  
        this.list.getScrollableNode().scrollTop -= scale;

Here we are scrolling equal to 3 lines at a time.

@Santhosh-Sellavel
Copy link
Collaborator

@luacmartins Any thoughts?

cc: @Expensify/design

@shawnborton
Copy link
Contributor

Did we introduce something recently that caused this bug?

@Santhosh-Sellavel
Copy link
Collaborator

I believe not!

@AndrewGable
Copy link
Contributor

AndrewGable commented Aug 31, 2022

Yes I believe we did introduce it recently with the react-native-web update. Not sure it was that code that broke it, but I believe it was around that timeframe.

@parasharrajat
Copy link
Member

parasharrajat commented Aug 31, 2022

There is nothing that could cause issues with this due to recent changes. The scroll is controlled via

this.list.getScrollableNode().scrollTop -= e.deltaY;
which uses browser scroll event and thus nothing in the code can affect it unless we change this handler.

As the handler was never changed, it is not a regression. The real thing is that may be Flatlist got performance boost somehow and thus the thread is able to scroll with better FPS.

@melvin-bot melvin-bot bot added the Overdue label Sep 9, 2022
@luacmartins
Copy link
Contributor

I'm not sure that this is actually an issue. I've been testing the scroll in our platforms and they seem fine to me. I vote we close this issue. Does anybody else disagree? @AndrewGable @shawnborton since you commented here.

@melvin-bot melvin-bot bot removed the Overdue label Sep 9, 2022
@shawnborton
Copy link
Contributor

I mostly use NewDot desktop app with an external mouse and scroll wheel and I haven't experienced this personally. So I would lean on someone to test the scenario in which is described above (trackpad + NewDot web) to see if it feels funky there still?

@luacmartins
Copy link
Contributor

IMO the scrolling speed seems fine on trackpad + web. I'm leaning towards closing this issue.

@laurenreidexpensify
Copy link
Contributor

Doublechecking if we should close https://expensify.slack.com/archives/C01SKUP7QR0/p1663676075990669

@laurenreidexpensify
Copy link
Contributor

Checking where to put this in testrail https://expensify.slack.com/archives/C01SKUP7QR0/p1668611021810339

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 17, 2022
@melvin-bot melvin-bot bot changed the title [$500] NewDot App scroll is 2-3x faster than other apps [HOLD for payment 2022-11-24] [$500] NewDot App scroll is 2-3x faster than other apps Nov 17, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.28-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-11-24. 🎊

@AndrewGable
Copy link
Contributor

Seems much better! Thanks for the fix.

@mountiny
Copy link
Contributor

+1 it is way nicer on mobile especially but web is great too

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 23, 2022
@laurenreidexpensify
Copy link
Contributor

@mountiny @Santhosh-Sellavel is this the PR that introduced the regression #11035?

@laurenreidexpensify
Copy link
Contributor

Asking about TestRail here

@laurenreidexpensify laurenreidexpensify removed the Awaiting Payment Auto-added when associated PR is deployed to production label Nov 24, 2022
@laurenreidexpensify laurenreidexpensify changed the title [HOLD for payment 2022-11-24] [$500] NewDot App scroll is 2-3x faster than other apps [PAID][HOLD for payment 2022-11-24] [$500] NewDot App scroll is 2-3x faster than other apps Nov 24, 2022
@laurenreidexpensify
Copy link
Contributor

Payment issued, let's close out the final steps in the checklist

@laurenreidexpensify laurenreidexpensify changed the title [PAID][HOLD for payment 2022-11-24] [$500] NewDot App scroll is 2-3x faster than other apps [PAID][$500] NewDot App scroll is 2-3x faster than other apps Nov 24, 2022
@Santhosh-Sellavel
Copy link
Collaborator

@laurenreidexpensify This PR Qualifies for an additional bonus $250 for both!

@Santhosh-Sellavel
Copy link
Collaborator

@mountiny @Santhosh-Sellavel is this the PR that introduced the regression #11035?

No that's not the PR.

@Santhosh-Sellavel
Copy link
Collaborator

I believe these are the PRs
Expensify/react-native-web#6 - Which included a fix to our fork
#10434 - Fork to App.

@Santhosh-Sellavel
Copy link
Collaborator

@laurenreidexpensify This PR Qualifies for an additional bonus $250 for both!

bump again!

@mountiny
Copy link
Contributor

@Santhosh-Sellavel Asked internally

@mountiny mountiny reopened this Nov 30, 2022
@laurenreidexpensify
Copy link
Contributor

Hey @Santhosh-Sellavel - payment of $750 was issued in Upwork 6 days ago, which was $500 for the job and the $250 bonus. Please recheck your account and let me know if this did not reflect on Upwork. Thanks

@Santhosh-Sellavel
Copy link
Collaborator

Sorry for the confusion, I missed that completely @laurenreidexpensify thanks for clarification! Also thanks @vit!

@Santhosh-Sellavel
Copy link
Collaborator

I believe these are the PRs
Expensify/react-native-web#6 - Which included a fix to our fork
#10434 - Fork to App.

This is offending PRs by the way @mountiny

@mountiny
Copy link
Contributor

Thanks @Santhosh-Sellavel and @laurenreidexpensify

I think in this case the offending PR is fine given it is RN update, that is very hard to catch every possible issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

10 participants