Added touchmove support to slider component by keul · Pull Request #5856 · plotly/plotly.js

keul · 2021-07-27T15:05:18Z

Note: the pull request is for the master branch, but I'm wondering if another one can be accepted for the old 1.x branch

See plotly#5855

alexcjohnson · 2021-08-04T20:56:19Z

@keul this looks good! Re: 1.x - we'd really rather keep our efforts focused on v2 except for security fixes. Is there something specific preventing you from updating to v2?

The one thing we should add is a test. Perhaps we could copy or extend this test:

plotly.js/test/jasmine/tests/sliders_test.js

Lines 470 to 508 in e27ca3c

    
           it('should respond to mouse clicks', function(done) { 
        
               var firstGroup = gd._fullLayout._infolayer.select('.' + constants.railTouchRectClass); 
        
               var firstGrip = gd._fullLayout._infolayer.select('.' + constants.gripRectClass); 
        
               var railNode = firstGroup.node(); 
        
               var touchRect = railNode.getBoundingClientRect(); 
        
               var originalFill = firstGrip.node().style.fill; 
        
               // Dispatch a click on the right side of the bar: 
        
               railNode.dispatchEvent(new MouseEvent('mousedown', { 
        
                   clientY: touchRect.top + 5, 
        
                   clientX: touchRect.left + touchRect.width - 5, 
        
               })); 
        
               expect(mockCopy.layout.sliders[0].active).toEqual(5); 
        
               var mousedownFill = firstGrip.node().style.fill; 
        
               expect(mousedownFill).not.toEqual(originalFill); 
        
               // Drag to the left side: 
        
               gd.dispatchEvent(new MouseEvent('mousemove', { 
        
                   clientY: touchRect.top + 5, 
        
                   clientX: touchRect.left + 5, 
        
               })); 
        
               var mousemoveFill = firstGrip.node().style.fill; 
        
               expect(mousemoveFill).toEqual(mousedownFill); 
        
               delay(100)() 
        
               .then(function() { 
        
                   expect(mockCopy.layout.sliders[0].active).toEqual(0); 
        
                   gd.dispatchEvent(new MouseEvent('mouseup')); 
        
                   var mouseupFill = firstGrip.node().style.fill; 
        
                   expect(mouseupFill).toEqual(originalFill); 
        
                   expect(mockCopy.layout.sliders[0].active).toEqual(0); 
        
               }) 
        
               .then(done, done.fail); 
        
           });

Note we have assets/touchEvent (and assets/mouseEvent) which were probably created after sliders_test but help to ensure these events are more realistic without making them harder to construct, like here in the shape drawing test:

plotly.js/test/jasmine/tests/draw_newshape_test.js

Lines 11 to 38 in e27ca3c

    
           var mouseEvent = require('../assets/mouse_event'); 
        
           var touchEvent = require('../assets/touch_event'); 
        
           var click = require('../assets/click'); 
        
           function drag(path, options) { 
        
               var len = path.length; 
        
               if(!options) options = { type: 'mouse' }; 
        
               if(options.type === 'touch') { 
        
                   touchEvent('touchstart', path[0][0], path[0][1], options); 
        
                   path.slice(1, len).forEach(function(pt) { 
        
                       touchEvent('touchmove', pt[0], pt[1], options); 
        
                   }); 
        
                   touchEvent('touchend', path[len - 1][0], path[len - 1][1], options); 
        
                   return; 
        
               } 
        
               mouseEvent('mousemove', path[0][0], path[0][1], options); 
        
               mouseEvent('mousedown', path[0][0], path[0][1], options); 
        
               path.slice(1, len).forEach(function(pt) { 
        
                   mouseEvent('mousemove', pt[0], pt[1], options); 
        
               }); 
        
               mouseEvent('mouseup', path[len - 1][0], path[len - 1][1], options);

keul · 2021-08-05T08:13:41Z

@alexcjohnson

Re: 1.x - we'd really rather keep our efforts focused on v2 except for security fixes. Is there something specific preventing you from updating to v2?

Nothing but not knowing the compatibility with server side modules. My use of plotly.js depends on the plotly Python module, so I try to keep the same version distributed with the lib itself.
But this is not a problem: I can continue providing my patch until I finally migrate to v2.

I'll take a look adding tests (not sure I'll be able to solve this before summer vacation). Thanks for references!

archmoj · 2021-08-05T13:09:36Z

Thanks for the PR.

Please create draftlogs/5856_add.md as described in this README.
Something like:

 - Add touch support to `slider` component [[#5856](https://github.com/plotly/plotly.js/pull/5856)], 
   with thanks to @keul for the contribution!

keul · 2021-08-05T16:07:38Z

test/jasmine/assets/touch_event.js

        target: el,
        clientX: x,
        clientY: y,
+        screenX: x,


I was forced to complete the signature of the event or simulating the touch event on the slider will generate a very strange behavior (slider was always moved on the start position).

Do we really need to add these? It looks the test passes without it.

@archmoj not on my machine (MacOS). Just tested again removing these lines and I have the failure i fixed adding this

BTW: I'm able to reproduce the issue also outside Jasmine/jsdom.

I'm 100% for removing this if you think it's just an issue on my Chrome… but seems really strange to me.

Thanks for testing.
LGTM 🏅
Over to @alexcjohnson

keul · 2021-08-05T16:09:17Z

@alexcjohnson I added the test.

But now there's another test that is failing: 5fe96ec#diff-09276d81835ab1fc83e8eb085e4faae2de13348f4be40367cb03276a3f651cbfR549

Seems like should respond to touch interactions is not really an isolated test. It starts to fails as soon as I add this new test, and it's back to normal when removing it, but I'm not able to understand what's wrong.

archmoj · 2021-08-05T16:16:16Z

@alexcjohnson I added the test.

But now there's another test that is failing: 5fe96ec#diff-09276d81835ab1fc83e8eb085e4faae2de13348f4be40367cb03276a3f651cbfR549

Seems like should respond to touch interactions is not really an isolated test. It starts to fails as soon as I add this new test, and it's back to normal when removing it, but I'm not able to understand what's wrong.

What happens if you add it to the end?

keul · 2021-08-05T16:23:30Z

What happens if you add it to the end?

@archmoj still fails

archmoj · 2021-08-05T16:34:50Z

The test passes on my machine.
To get all the greens, please fetch upstream/master in your fork, then pull and merge it into this branch.

keul · 2021-08-05T16:51:54Z

@archmoj really sorry for the noise… It works now 🙇

alexcjohnson

Love it. 💃

archmoj · 2021-08-05T18:54:22Z

We will wait for @nicolaskruchten before merging.

Added touchmove support to slider component

e9953c1

See plotly#5855

keul force-pushed the slider-touch-support branch from d251613 to e9953c1 Compare July 27, 2021 15:10

archmoj added status: reviewable community community contribution feature something new labels Jul 27, 2021

archmoj requested a review from alexcjohnson July 27, 2021 19:30

Added test to cover plotly#5855

5fe96ec

keul commented Aug 5, 2021

View reviewed changes

Merge branch 'master' into slider-touch-support

a5abb7f

alexcjohnson approved these changes Aug 5, 2021

View reviewed changes

archmoj requested a review from nicolaskruchten August 5, 2021 18:54

archmoj merged commit 13944ae into plotly:master Aug 24, 2021

keul deleted the slider-touch-support branch August 24, 2021 16:39

archmoj added this to the v2.4.0 milestone Aug 24, 2021

Uh oh!

Conversation

keul commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcjohnson commented Aug 4, 2021

Uh oh!

keul commented Aug 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

archmoj commented Aug 5, 2021

Uh oh!

keul Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

archmoj Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

keul Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

keul Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

archmoj Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

keul commented Aug 5, 2021

Uh oh!

archmoj commented Aug 5, 2021

Uh oh!

keul commented Aug 5, 2021

Uh oh!

archmoj commented Aug 5, 2021

Uh oh!

keul commented Aug 5, 2021

Uh oh!

alexcjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

archmoj commented Aug 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

keul commented Jul 27, 2021 •

edited

Loading

keul commented Aug 5, 2021 •

edited

Loading