Add touch support in our carousel #25776

Johann-S · 2018-03-03T21:05:23Z

Add unit test
Documentation

XhmikosR · 2018-03-03T22:37:27Z

Its quite a big addition... Can't we do the basic stuff ourselves?

XhmikosR · 2018-03-03T22:38:04Z

_includes/scripts.html

@@ -2,6 +2,8 @@
 <script>window.jQuery || document.write('<script src="{{ site.baseurl }}/assets/js/vendor/jquery-slim.min.js"><\/script>')</script>

 <script src="{{ site.baseurl }}/assets/js/vendor/popper.min.js"{% if site.github %} integrity="{{ site.cdn.popper_hash }}" crossorigin="anonymous"{% endif %}></script>
+<script src="{{ site.baseurl }}/assets/js/vendor/hammer.min.js"{% if site.github %} integrity="{{ site.cdn.hammer_hash }}" crossorigin="anonymous"{% endif %}></script>


You need to add it in package.json (uglify) too for the docs package.

It's strange but Popper isn't in our doc-minify task 🤔

True, indeed. Just leave it as is and I'll give it a go in the next days and let you know. :)

Johann-S · 2018-03-03T23:20:38Z

I don't think it's a big change here, we add an optionnal lib, if folks need a touch support that's all

XhmikosR · 2018-03-03T23:24:40Z

Sure, but can't we have the basic functionality without a 20 KB file? I recall you had a branch in the past.

Johann-S · 2018-03-03T23:26:52Z

This won't be added in our dist files, so we won't add 20 kb

I'm not in favor of adding logics about touch detection and swipe detection when they are libs which can do that better than me

XhmikosR · 2018-03-03T23:30:18Z

I agree on that, but then one would say why don't you support jQuery mobile or whatever other lib there's out there.

Anyway, I'm not against this, I just want us to think it through.

Johann-S · 2018-03-03T23:32:00Z

I think HammerJS is a good choice, Angular chose HammerJS too

jacobmllr95 · 2018-03-04T10:57:56Z

js/src/carousel.js

      if (this._config.keyboard) {
        $(this._element)
          .on(Event.KEYDOWN, (event) => this._keydown(event))
      }

+      if (touchSupported) {
+        this.hammer.on(Event.SWIPELEFT, () => this.next())


Isn't here a check quired if hammer support is enabled or the hammer reference exists to prevent a reference error? 🤔

Yep that's true nice catch 👍

XhmikosR · 2018-04-10T07:20:39Z

js/tests/index.html

@@ -20,11 +20,15 @@
      }())
    </script>
    <script src="../../assets/js/vendor/popper.min.js"></script>
+    <script src="../../assets/js/vendor/hammer.min.js"></script>


Maybe load this from node_modules?

Scratch that, it's a peerDependency. How about using the CDN instead of adding yet one more file?

XhmikosR · 2018-04-10T07:40:42Z

js/tests/karma.conf.js

@@ -17,6 +17,8 @@ module.exports = (config) => {
    files: [
      jqueryFile,
      'assets/js/vendor/popper.min.js',
+      'assets/js/vendor/hammer.min.js',


This shouldn't be needed.

Johann-S · 2018-10-16T10:01:24Z

@patrickhlauke everything is fine now, can you confirm it ? Thanks 👍

js/src/carousel.js

patrickhlauke · 2018-10-16T12:27:24Z

i can have one last actual live test later tonight on my phones and Surface, but conceptually this all looks good to me now

Johann-S · 2018-10-16T12:52:04Z

It works fine for me 👍

/CC @XhmikosR if you want to do some tests too

patrickhlauke · 2018-10-16T13:38:29Z

https://deploy-preview-25776--twbs-bootstrap4.netlify.com/docs/4.1/components/carousel/ seems to be working fine now (tested there as i've not got my build/test environment all set up again at the moment) ... but man, the aggressive caching/service worker really gets in the way of quickly checking changes out sometimes...

possibly more related to netlify here: on IE11 and Edge, I found that the carousels are unresponsive for quite a long time after page load (unless you kick them into action if there are the "<" / ">" controls). it seems the browser is waiting for some file to load from the network, and only once it gives up does it run the onload stuff.

XhmikosR · 2018-10-16T13:50:30Z

There is actually no service worker on netlify; it doesn't run the production scripts on purpose.

patrickhlauke · 2018-10-16T14:05:03Z

There is actually no service worker on netlify; it doesn't run the production scripts on purpose.

i found i had to force refresh (and in the case of Firefox, go in and delete session storage etc) while testing. not quite sure why then...aggressive caching? i see pwa.js is loaded as well if that means anything...i'm actually not sure if this is related to service worker or not.

in any case, it seems to be the carbon ads json that is never loaded (shows as "(pending)" in the F12 tools' network tab). only once the browser gives up trying to load it is the JS on page load actually executed.

anyway, that's all tangential. the carousel swipe thing now seems to work nicely

XhmikosR · 2018-10-16T14:24:35Z

Correction, the service worker is being there albeit broken on dev mode. I definitely need to finish #26868 and #27320.

I'm gonna look into this branch later and get back to you guys.

XhmikosR · 2018-10-20T08:17:07Z

This no longer works for me with Firefox 63 devtools and say S9 emulation. It works fine with Chrome though.

Johann-S · 2018-10-20T08:32:44Z

@XhmikosR that's strange it works perfectly on my Android with Firefox

XhmikosR · 2018-10-20T08:42:00Z

Well a real touch device is not the same as the emulator :) That being said I don't know if we should even care about it. I just thought I'd report it since Firefox is my main browser and this worked in the previous patches.

Johann-S · 2018-10-20T09:37:38Z

I think we should test using BrowserStack here

patrickhlauke · 2018-10-20T10:26:21Z

The reason why it's not working in Firefox's device emulation is that we check for navigator.maxTouchPoints (part of the pointer events spec), but device emulation doesn't fake this value. So touchSupported evaluates to false in the code, and none of the swipe code is attached. tl;dr: it's the fault of Firefox's emulation.

https://patrickhlauke.github.io/touch/tests/pointer-multitouch-detect.html

[edit: filed a bug here: Bug 1500672 - Responsive Design Mode with touch simulation: navigator.maxTouchPoints still zero]

XhmikosR · 2018-10-20T11:33:22Z

All right, let's merge this then. Great work everyone!

Johann-S added js feature v4 labels Mar 3, 2018

Johann-S requested review from mdo, XhmikosR, patrickhlauke and bardiharborow March 3, 2018 21:05

Johann-S force-pushed the v4-carousel-hammer branch 2 times, most recently from 6951e04 to e91aee5 Compare March 3, 2018 21:15

XhmikosR reviewed Mar 3, 2018

View reviewed changes

Johann-S force-pushed the v4-carousel-hammer branch 2 times, most recently from a8b3276 to 89fcc36 Compare March 4, 2018 00:05

jacobmllr95 reviewed Mar 4, 2018

View reviewed changes

Johann-S force-pushed the v4-carousel-hammer branch 2 times, most recently from 502272d to bef1560 Compare March 4, 2018 16:28

Johann-S changed the title ~~WIP: Add touch support on our carousel with Hammer~~ Add touch support on our carousel with Hammer Mar 4, 2018

Johann-S mentioned this pull request Mar 9, 2018

v4 - Touch gesture support for carousel #17118

Closed

XhmikosR reviewed Apr 10, 2018

View reviewed changes

Johann-S force-pushed the v4-carousel-hammer branch 2 times, most recently from 1c34111 to 5823397 Compare April 10, 2018 07:50

patrickhlauke reviewed Oct 16, 2018

View reviewed changes

js/src/carousel.js Outdated Show resolved Hide resolved

Johann-S force-pushed the v4-carousel-hammer branch 3 times, most recently from 8bcaeb3 to 652161f Compare October 16, 2018 11:46

avoid drag img

098d089

Johann-S force-pushed the v4-carousel-hammer branch from 652161f to 098d089 Compare October 16, 2018 11:59

Merge branch 'v4-dev' into v4-carousel-hammer

ab89885

Merge branch 'v4-dev' into v4-carousel-hammer

f02a3a0

mdo mentioned this pull request Oct 20, 2018

v4.2.0 shiplist #26952

Closed

XhmikosR changed the title ~~Add touch support on our carousel~~ Add touch support in our carousel Oct 20, 2018

Merge branch 'v4-dev' into v4-carousel-hammer

9ed3bd5

XhmikosR merged commit b1dc6b3 into v4-dev Oct 20, 2018

XhmikosR deleted the v4-carousel-hammer branch October 20, 2018 12:32

MartijnCuppens mentioned this pull request Oct 28, 2018

Not able to scroll with touch support for carousels #27531

Closed

musaffa mentioned this pull request Apr 3, 2019

Touch gesture support Carousel ember-bootstrap/ember-bootstrap#613

Open

taion mentioned this pull request Apr 22, 2019

Add touch support for carousels react-bootstrap/react-bootstrap#3683

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add touch support in our carousel #25776

Add touch support in our carousel #25776

Johann-S commented Mar 3, 2018 •

edited

Loading

XhmikosR commented Mar 3, 2018

XhmikosR Mar 3, 2018 •

edited

Loading

Johann-S Mar 3, 2018

XhmikosR Mar 4, 2018

Johann-S commented Mar 3, 2018

XhmikosR commented Mar 3, 2018

Johann-S commented Mar 3, 2018

XhmikosR commented Mar 3, 2018

Johann-S commented Mar 3, 2018

jacobmllr95 Mar 4, 2018

Johann-S Mar 4, 2018

XhmikosR Apr 10, 2018

XhmikosR Apr 10, 2018

Johann-S Apr 10, 2018

XhmikosR Apr 10, 2018

Johann-S commented Oct 16, 2018

patrickhlauke commented Oct 16, 2018

Johann-S commented Oct 16, 2018

patrickhlauke commented Oct 16, 2018

XhmikosR commented Oct 16, 2018

patrickhlauke commented Oct 16, 2018

XhmikosR commented Oct 16, 2018

XhmikosR commented Oct 20, 2018

Johann-S commented Oct 20, 2018

XhmikosR commented Oct 20, 2018

Johann-S commented Oct 20, 2018

patrickhlauke commented Oct 20, 2018 •

edited

Loading

XhmikosR commented Oct 20, 2018

Add touch support in our carousel #25776

Add touch support in our carousel #25776

Conversation

Johann-S commented Mar 3, 2018 • edited Loading

XhmikosR commented Mar 3, 2018

XhmikosR Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johann-S commented Mar 3, 2018

XhmikosR commented Mar 3, 2018

Johann-S commented Mar 3, 2018

XhmikosR commented Mar 3, 2018

Johann-S commented Mar 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johann-S commented Oct 16, 2018

patrickhlauke commented Oct 16, 2018

Johann-S commented Oct 16, 2018

patrickhlauke commented Oct 16, 2018

XhmikosR commented Oct 16, 2018

patrickhlauke commented Oct 16, 2018

XhmikosR commented Oct 16, 2018

XhmikosR commented Oct 20, 2018

Johann-S commented Oct 20, 2018

XhmikosR commented Oct 20, 2018

Johann-S commented Oct 20, 2018

patrickhlauke commented Oct 20, 2018 • edited Loading

XhmikosR commented Oct 20, 2018

Johann-S commented Mar 3, 2018 •

edited

Loading

XhmikosR Mar 3, 2018 •

edited

Loading

patrickhlauke commented Oct 20, 2018 •

edited

Loading