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

[DTRA] Maryia/feat: vertical scroll behavior updates for isVerticalScrollEnabled functionality when disabled #1628

Merged

Conversation

maryia-deriv
Copy link
Contributor

@maryia-deriv maryia-deriv commented Aug 9, 2024

Dtrader V2 changes for isVerticalScrollEnabled === false:

With isVerticalScrollEnabled={false}, the chart scroll behavior will be now as follows:

  • The chart internal scroll is blocked when the page is scrolled vertically outside Y-axis or X-axis using toggleXScrollBlock method that has been added to the flutter-chart controller.
  • Max vertical scroll is activated (the page will be scrolled to the very top/bottom) when a user swipes quickly up or down, i.e. scroll delta exceeds 10px.
  • Slow vertical scroll is made smoother.

Deriv-app PR: binary-com/deriv-app#16434
Flutter-chart PR: https://github.com/regentmarkets/flutter-chart/pull/327/

@maryia-deriv maryia-deriv changed the title [DTRA] Maryia/test: improve cV2 chart scroll behavior [WIP] [DTRA] Maryia/test: improve V2 chart scroll behavior [WIP] Aug 11, 2024
@maryia-deriv maryia-deriv changed the title [DTRA] Maryia/test: improve V2 chart scroll behavior [WIP] [DTRA] Maryia/feat: update chart scroll behavior with isVerticalScrollEnabled prop enabled Aug 11, 2024
@maryia-deriv maryia-deriv marked this pull request as ready for review August 12, 2024 00:34
@maryia-deriv maryia-deriv changed the title [DTRA] Maryia/feat: update chart scroll behavior with isVerticalScrollEnabled prop enabled [DTRA] Maryia/feat: update chart scroll behavior with isVerticalScrollEnabled prop set to false Aug 12, 2024
@maryia-deriv maryia-deriv changed the title [DTRA] Maryia/feat: update chart scroll behavior with isVerticalScrollEnabled prop set to false [DTRA] Maryia/feat: update vertical scroll behavior disabled using isVerticalScrollEnabled prop Aug 12, 2024
@maryia-deriv maryia-deriv changed the title [DTRA] Maryia/feat: update vertical scroll behavior disabled using isVerticalScrollEnabled prop [DTRA] Maryia/feat: update vertical scroll behavior when disabled with isVerticalScrollEnabled prop Aug 12, 2024
top: this.touchValues.deltaYTotal,
behavior: 'smooth',
});
this.scrollChartParentOnTouchTimer = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside of timeout timer can be cleared only by undefined assignment or this will work too:⤵️? clearTimeout(this.scrollChartParentOnTouchTimer);

Copy link
Contributor Author

@maryia-deriv maryia-deriv Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't want to clear the timer inside it because it has already been executed, it won't be needed.
the purpose of this.scrollChartParentOnTouchTimer = undefined; (id removal) is to ensure that we initiate the next setTimeout execution only after the current one has finished execution (not let the if condition be satisfied).

top: screenY - Number(this.touchValues.yOnTouchEnd ?? this.touchValues.y),
const isForcedScrollArea = x < nonScrollableAreaWidth;
const shouldForceMaxScroll =
Math.abs(Number(this.touchValues.deltaYTotal)) > 10 && e.type === 'touchend';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes we get touchcancel instead of touchend, do we need to handle this case too?

Copy link
Contributor Author

@maryia-deriv maryia-deriv Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, we don't need to do that. also, we won't get touchcancel here ever because we don't trigger onTouch action for this event type.

Math.abs(Number(this.touchValues.deltaYTotal)) > 10 && e.type === 'touchend';
const stopChartPagination = () => {
// calling _scrollAnimationController.stop() in flutter-chart, value 1 doesn't affect the scale:
this.flutterChart?.app.scale(1);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this triggers the following method in flutter-chart which allows to stop chart pagination when we are scrolling:
Screenshot 2024-08-13 at 5 08 11 PM

@maryia-deriv maryia-deriv changed the title [DTRA] Maryia/feat: update vertical scroll behavior when disabled with isVerticalScrollEnabled prop [DTRA] Maryia/feat: update vertical scroll behavior when using isVerticalScrollEnabled prop with false (disabled) value Aug 15, 2024
@maryia-deriv maryia-deriv changed the title [DTRA] Maryia/feat: update vertical scroll behavior when using isVerticalScrollEnabled prop with false (disabled) value [DTRA] Maryia/feat: vertical scroll behavior updates for isVerticalScrollEnabled (false) functionality Aug 15, 2024
@maryia-deriv maryia-deriv changed the title [DTRA] Maryia/feat: vertical scroll behavior updates for isVerticalScrollEnabled (false) functionality [DTRA] Maryia/feat: vertical scroll behavior updates for isVerticalScrollEnabled functionality when disabled Aug 15, 2024
Copy link

Preview Link: https://pr-1628-dtra-flutt.smartcharts-preview.pages.dev

Name Result
Build status Completed ✅
Preview URL Visit Preview
Action URL Visit Action

@balakrishna-deriv balakrishna-deriv merged commit 069c469 into deriv-com:master Aug 28, 2024
3 checks passed
@DerivFE
Copy link
Collaborator

DerivFE commented Aug 28, 2024

🎉 This PR is included in version 2.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants