search

seiyria / bootstrap-slider

A slider control for Bootstrap 3 & 4.
http://seiyria.github.io/bootstrap-slider/
Other
3k stars 1.14k forks source link

Fix touch drag on mobile devices #899

Closed jespirit closed 5 years ago

jespirit commented 5 years ago

This pull request addresses these issues:

Page scrolling: https://github.com/seiyria/bootstrap-slider/issues/639 Passive listeners: https://github.com/seiyria/bootstrap-slider/issues/842

Tooltips not displaying on touch-enabled mobile devices: https://github.com/seiyria/bootstrap-slider/issues/513 https://github.com/seiyria/bootstrap-slider/issues/765

You can check my project page to test the latest changes from this branch: https://jespirit.github.io/bootstrap-slider/. I've tested it mobile devices using Chrome's Device Mode.

I also tested it on my Samsung A5 (2017), Android 8.0.0

Unit tests...

Edit:

Pull Requests

Please accompany all pull requests with the following (where appropriate):

jespirit commented 5 years ago

Some of the touch event unit tests check whether a event listener was called more than once. What's the best way to handle that in an asynchronous way?

it("slider should not have duplicate events after calling 'refresh'", function() {
  touch.initEvent("touchstop");
  flag = 0;

  testSlider.on('slideStop', function() {
    flag += 1;
  });
  testSlider.slider('refresh');
  testSlider.data('slider')._mouseup(mouse);

  expect(flag).toEqual(1);
});

I used a fake 'temp' event with a temporary handler that I call with .trigger().

it("slider should not have duplicate events after calling 'refresh'", function(done) {
  flag = 0;
  var tempHandler = function() {
    // Remove this handler
    testSlider.off('temp', tempHandler);
    expect(flag).toBe(1);
    done();
  };

  testSlider.on('slideStop', function() {
    flag++;
  });

  sliderElem.dispatchEvent(touchStart);
  sliderElem.dispatchEvent(touchMove);
  sliderElem.dispatchEvent(touchEnd);
  testSlider.slider('refresh');

  testSlider.on('temp', tempHandler);
  testSlider.trigger('temp');
});
rovolution commented 5 years ago

@rovolution Is this code meant for browser compatibility and performance?

Its been a while, but I believe the original intention was to prevent duplicate events from being triggered...I vaguely remember it, but there was some case where a device was binding to both the touch and the regular mouse events, and was thus causing the same slider events to be emitted multiple times.

Fenny commented 5 years ago

Any updates?

jespirit commented 5 years ago

Any updates?

I have to write some unit tests and do more manual testing before I can merge this. I'll try to schedule some time to work on this hopefully next week because I know touch features are kind of broken.

danimiba commented 5 years ago

Is there any news on this?

Fenny commented 5 years ago

Please fix, we are thankful 😊

jespirit commented 5 years ago

Sorry, I've been busy with school. I'll have some time tomorrow to work on it.

This is a major pull request and there are a lack of unit tests for touch capability. I don't want to publish code that isn't tested properly. I don't really have a way of knowing whether this works on real devices. I don't have access to an iPhone or iPad.

seiyria commented 5 years ago

I'll work on setting up deploy previews so we can test this more easily.

seiyria commented 5 years ago

Next time this PR is pushed to, there should be a deploy preview. I can help validate this on my devices and hopefully some in this thread can help as well.

Thanks!

rovolution commented 5 years ago

@seiyria that will be awesome! I just did a quick read on Netlify and it looks super cool!

Fenny commented 5 years ago

That would be great, a lot of ecommerce shops run on this plugin.

rovolution commented 5 years ago

@jespirit did you try rebasing this topic branch against the current master branch? i think that may be why the netifly preview is failing cc: @seiyria

jespirit commented 5 years ago

I rebased. It was messy though. I'm glad it didn't break any tests.

rovolution commented 5 years ago

@jespirit is this good to go? did you QA the deploy preview in Netlify?

https://deploy-preview-899--bootstrap-slider.netlify.com/

Also remember to remove the console.logs from the source code

jespirit commented 5 years ago

It definitely passes the manual QA. I've only tested on Google Chrome Dev tools and my Samsung A5.

I just need to add a few more basic tests (plus remove debug statements) then I think it's good to merge.

omarseddik commented 5 years ago

@jespirit Replace blue with blur ! Thanks for fixing mobile bug

screen shot 2019-02-12 at 00 47 48
jespirit commented 5 years ago

@jespirit Replace blue with blur ! Thanks for fixing mobile bug

screen shot 2019-02-12 at 00 47 48

Nice catch. Thanks for that.

I will have time on Wednesday to write the tests and soon after merge it.

jespirit commented 5 years ago

Done.

codeclimate would not be happy about the unit tests