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

Closes issue #58 #59

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Closes issue #58 #59

wants to merge 15 commits into from

Conversation

Siutan
Copy link

@Siutan Siutan commented Aug 18, 2023

Closes #58

Changes made:

  • Adjusted handleDragging() function to check for distance dragged.
  • Added dragThreshold variable to set the intensity of the drag effect (Default is 50).
  • Added tapped variable to check if distance of drag < threshold.

Notes:

dragThreshold is exported and can be used like:

<Fullpage dragThreshold={100}>
    ...
</Fullpage>

The quick way to fix this is also to do the following in FullpageController.svelte:

const handleDragEnd = () => {
        const hasScrolledUp = dragStartScroll > fullpage.scrollTop - 1 // append - 1 to offset the tap, thus disabling scroll
    ...
}

I was unsure if this would bring up more issues/outliers later, so I didn't implement it this way.
I also found it easier to implement the sensitivity (dragThreshold) using the handleDragging() function as opposed to the above fix.

Copy link
Owner

@Hejtmus Hejtmus left a comment

Choose a reason for hiding this comment

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

Generally I like it, but I don't like introducing demanding calculations. The other solution you have suggested did the trick, but I could reproduce the bug when rapidly dragging and then clicking. So I actually think this is better solution, if you could somehow simplify tat threshold calculation, it would be great. But if that's over your time limit, I can accept your other solution as temporary fix, and later fix it properly. Thanks for PR!

Copy link
Owner

Choose a reason for hiding this comment

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

Looks fine, but could you explain, why did you pick 50 as default value?

Copy link
Author

Choose a reason for hiding this comment

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

50 felt the most natural when swiping. But that was my personal preference and other users might want something different so I exported it so users can choose.

Copy link
Owner

Choose a reason for hiding this comment

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

Well, I like that you are thinking of people that would like to customize this, but I think that something that distinguishes whether is something tap or swipe should be deterministic and not adjustable. It shouldn't be like some concrete event is on one page tap and on another swipe.

src/lib/Fullpage.svelte Outdated Show resolved Hide resolved
src/lib/FullpageController.svelte Outdated Show resolved Hide resolved
src/lib/FullpageController.svelte Outdated Show resolved Hide resolved
src/lib/FullpageController.svelte Outdated Show resolved Hide resolved
src/lib/FullpageController.svelte Outdated Show resolved Hide resolved
src/lib/FullpageController.svelte Outdated Show resolved Hide resolved
src/lib/FullpageController.svelte Outdated Show resolved Hide resolved
src/lib/FullpageController.svelte Outdated Show resolved Hide resolved
duration: 0
})
const distance = Math.abs(event.clientY - dragPosition);
const threshold = distance > (fullpage.clientHeight / dragThreshold);
Copy link
Owner

Choose a reason for hiding this comment

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

I think here is too much math done for event that's fired every time user drags. Especially division is pretty demanding, it would be fine to mitigate it.

Copy link
Author

@Siutan Siutan Aug 20, 2023

Choose a reason for hiding this comment

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

Maybe we can improve the performance by using squared distances as opposed to absolutes, for example:

const distance = Math.pow(event.clientY - dragPosition, 2);
const threshold = Math.pow(fullpage.clientHeight / dragThreshold, 2);
const isDrag = distance > threshold;

This way we can avoid the square root calculation involved in computing the actual distances. The side-effect of this is maybe a few edge cases where user inputs are wrongly identified.

Another option is to use bitwise operations, an example would be:

const distance= (event.clientY - dragPosition) ** 2;
const threshold = (fullpage.clientHeight / dragThreshold) ** 2;
const isDrag = distance > threshold;

This optimization works because its only interested in whether one distance is greater than another, and you don't need the actual values; however, I believe the performance gain opposed to this will not be significant as this is not a large enough calculation to warrant it.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for trying for optimization,
but I meant so meting different, like changing semantics of dragThreshold to entirely mitigate division. Actually calculating squared distances would not speed up calculations as it is defined not by power and root, but rather as negation of negative value, and you can see it in V8 code. And FYI '**' operator is actually not bitwise operator, but just expressive writing of power.

@Hejtmus
Copy link
Owner

Hejtmus commented Aug 19, 2023

If you'll be committing some code from code review, please use conventional commits, sorry for not mentioning it in project contribution guidelines.

Siutan and others added 10 commits August 20, 2023 11:23
formatting

Co-authored-by: Filip Holčík <filip.holcik.official@gmail.com>
removed console log

Co-authored-by: Filip Holčík <filip.holcik.official@gmail.com>
formatting

Co-authored-by: Filip Holčík <filip.holcik.official@gmail.com>
formatting

Co-authored-by: Filip Holčík <filip.holcik.official@gmail.com>
formatting

Co-authored-by: Filip Holčík <filip.holcik.official@gmail.com>
fixed reference order of statements

Co-authored-by: Filip Holčík <filip.holcik.official@gmail.com>
removed unnecessary statement

Co-authored-by: Filip Holčík <filip.holcik.official@gmail.com>
formatting

Co-authored-by: Filip Holčík <filip.holcik.official@gmail.com>
formatting

Co-authored-by: Filip Holčík <filip.holcik.official@gmail.com>
formatting

Co-authored-by: Filip Holčík <filip.holcik.official@gmail.com>
Copy link
Owner

@Hejtmus Hejtmus left a comment

Choose a reason for hiding this comment

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

Now I'm starting to not like this solution, it's over complicated and it could pollute code, which will require breaking change to clean (I mean that export of DragThreshold). When I was working on version 1.0.0, I was trying to minimize number of needed exports props needed for customization, but to keep level of customization high. Adding prop for decision whether something is tap or drag is overkill, and I thing it is generally decidable with simple heuristic.
I admire you will to fix this bug, and also your time investment, but I don't think this solution should be merged. I'd rather merge that other 1 line heuristic fix for now, and later, when I'll have time, I'll address this issue. Currently I'm busy to experiment with ways of fixing this, but please, keep this PR open, if I later find out, this is the best solution, so you cold heave credit on this. Please could you create another PR with that other solution?

Thank you very much,
Filip.

duration: 0
})
const distance = Math.abs(event.clientY - dragPosition);
const threshold = distance > (fullpage.clientHeight / dragThreshold);
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for trying for optimization,
but I meant so meting different, like changing semantics of dragThreshold to entirely mitigate division. Actually calculating squared distances would not speed up calculations as it is defined not by power and root, but rather as negation of negative value, and you can see it in V8 code. And FYI '**' operator is actually not bitwise operator, but just expressive writing of power.

Copy link
Owner

Choose a reason for hiding this comment

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

Well, I like that you are thinking of people that would like to customize this, but I think that something that distinguishes whether is something tap or swipe should be deterministic and not adjustable. It shouldn't be like some concrete event is on one page tap and on another swipe.

@Siutan
Copy link
Author

Siutan commented Aug 22, 2023

Okay, thank you for giving me a better understanding both on the direction of this library and on the v8 implementation. I will create a new pull request for the other solution when I have time this week.
Thank you,
Mukil

@Hejtmus
Copy link
Owner

Hejtmus commented Aug 25, 2023

Hi @Siutan,
today I'll be committing some code to this lib, if you are busy and you want, I can commit your other solution and tag you as co-author, so you won't lose your credit. I can wait with release for your answer.

Best,
Filip.

@Siutan
Copy link
Author

Siutan commented Aug 25, 2023

Hi @Hejtmus,

Please go ahead and include the change with your commit, unfortunately I have not had time to work on it.

Thanks,
Mukil

@Hejtmus
Copy link
Owner

Hejtmus commented Aug 25, 2023

Now I found special edge case in that one line solution, that scrolls up any section, that is below other section and doesn't have slides, so I have to find another solution to #58. Hopefully next week I have had finished work for my clients, so I can figure out solution with no side effects.
Thanks again for report and your will and patience to fix #58, I will inform you about state of the issue.

Have a nice day,
Filip.

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

Successfully merging this pull request may close these issues.

FullPageSlide scrolls down on mobile when tapped
2 participants