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

Scroll up call refresh handler every time in multiple.html demo using 0.1.16 #69

Closed
flying3615 opened this issue Apr 5, 2019 · 7 comments
Assignees

Comments

@flying3615
Copy link

flying3615 commented Apr 5, 2019

Bug report

In our mobile app, after upgrading from 0.1.14 to 0.1.16,
the scrollbar will trigger the onRefresh handler every time users touch and drag up even though the view is not at the very top.

I believe the code here makes trouble, the this.mainElement.scrollTop always return 0 each time scroll to be dragged up. However, using the old way, it works fine.

shouldPullToRefresh: function shouldPullToRefresh() {
      return typeof this.mainElement === 'string' ? !document.querySelector(this.mainElement).scrollTop : this.mainElement && !this.mainElement.scrollTop;
      // return !window.scrollY
    },

Current behavior:
Calling refresh handler every time when scrolling up the page.
Expected behavior:
Not calling refresh handler when each time scroll up.

JSFiddle URL for the demo with bug:
This bug can be reproduced by tweaking the demo which is multiple.html, looks like the demo still using the old version, by replacing the pulltorefresh.js with the latest 0.1.16, the issue can be observed
https://jsfiddle.net/bqc3jnds/
Browsers affected:
Chrome dev tool in mobile mode and real devices

@faelsoto
Copy link
Member

faelsoto commented Apr 5, 2019

Hey, could you please check the JSFiddle URL and make sure it's working fine? I can't make it work so I can't replicate the issue.

@flying3615
Copy link
Author

Sorry, I'm not quite familiar with using JSFiddle, but it can be reproduced easily by using the 0.1.16 code for the demo, multiple.html.
After that, open chrome dev tool, in the mobile mode, drag down the page and drag up again, the issue can be observed.

@flying3615
Copy link
Author

I'm not sure JSFiddle the running result can simulate mobile mode, this issue only happens by using the mouse click drag to simulate hand drag, not the mouse wheel

@flying3615
Copy link
Author

Also in the readme, it said as below, but in the 0.1.16 the approach has been changed.

shouldPullToRefresh (function) Which condition should be met for pullToRefresh to trigger? 
— Defaults to !window.scrollY

@crux153
Copy link
Contributor

crux153 commented Apr 5, 2019

I would also like this issue to get addressed. Currently, I'm providing previous default value manually.

pullToRefresh.init({
    shouldPullToRefresh: () => !window.scrollY
});

@pateketrueke
Copy link
Collaborator

I need time to investigate this further, so much thanks for your feedback!

@pateketrueke pateketrueke self-assigned this Apr 5, 2019
@pateketrueke
Copy link
Collaborator

Gotcha, definitely this will depend on how you're building your layout, the default behaviour will not satisfy all needs as layouts can be built in many different ways.

We can go back to the previous default for shouldPullToRefresh but that will also break other's users setups and such.

The best solution so far is providing a callback that fits to your layout logic.

So much thanks for your feedback!

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

No branches or pull requests

4 participants