Improved event handling #379

alexreardon · 2018-03-09T05:50:09Z

Closes #375

alexreardon · 2018-03-11T09:51:23Z

.storybook/config.js

+configure(loadStories, module);
+
+// console.log('Adding some window event handlers to the story');


remove debug code

alexreardon · 2018-03-11T09:51:47Z

docs/guides/how-we-use-dom-events.md

@@ -0,0 +1,135 @@
+# How we use DOM events


A completely new page explaining in painful detail how we manage events

alexreardon · 2018-03-11T09:52:15Z

docs/patterns/multi-drag.md

@@ -0,0 +1,105 @@
+# Mutli drag pattern


Remove this file from the PR 😊

alexreardon · 2018-03-11T09:52:42Z

src/debug/timings.js

@@ -0,0 +1,48 @@
+// @flow


I keep writing this code all the time so I decided to bake it in

alexreardon · 2018-03-11T09:52:55Z

src/view/drag-handle/drag-handle.jsx

@@ -21,7 +21,9 @@ import createMouseSensor from './sensor/create-mouse-sensor';
 import createKeyboardSensor from './sensor/create-keyboard-sensor';
 import createTouchSensor from './sensor/create-touch-sensor';

-const getFalse: () => boolean = () => false;
+const preventHtml5Dnd = (event: DragEvent) => {


now actually doing the right thing

alexreardon · 2018-03-11T09:53:14Z

src/view/drag-handle/drag-handle-types.js

@@ -36,8 +36,7 @@ export type DragHandleProps = {|

  // Stop html5 drag and drop
  draggable: boolean,
-  onDragStart: () => boolean,
-  onDrop: () => boolean


onDrop is not actually needed now that we are doing the correct thing in onDragStart

Should all references to onDrop be removed or do they refer to different things? Namely the onDrop prop in the Callbacks type above and its usages.

alexreardon · 2018-03-11T09:54:19Z

src/view/drag-handle/sensor/create-keyboard-sensor.js

  };

-  const windowBindings = {
+  const windowBindings: EventBinding[] = [


new way of bindings events that is WAY more consistent between sensors

alexreardon · 2018-03-11T09:55:07Z

src/view/drag-handle/sensor/create-mouse-sensor.js

@@ -53,8 +59,9 @@ export default ({
  const stopDragging = (fn?: Function = noop, shouldBlockClick?: boolean = true) => {
    schedule.cancel();
    unbindWindowEvents();
+    mouseDownMarshal.reset();


not sure if we need a .reset but i'll explain later

alexreardon · 2018-03-11T09:56:40Z

src/view/drag-handle/sensor/create-mouse-sensor.js

  };

  const onMouseDown = (event: MouseEvent): void => {
+    if (mouseDownMarshal.isHandled()) {


a new mechanism to avoid parents using the same event without needing to call .preventDefault or stop the event

alexreardon · 2018-03-12T00:31:59Z

src/view/drag-handle/util/bind-events.js

+  bindings.forEach((binding: EventBinding) => {
+    const options: Object = {
+      ...sharedOptions,
+      ...binding.options,


bindings can have their own overrides

…o event-handling

Blasz

Looks good. I have a couple of minor comments, none of which are blocking.

Blasz · 2018-03-12T02:56:19Z

.storybook/config.js

+//     defaultPrevented: ${event.defaultPrevented}
+//   `));
+// });


Should this be left in? Will it be uncommented later on?

Blasz · 2018-03-12T02:57:57Z

README.md

@@ -1256,7 +1262,7 @@ The `children` function is also provided with a small amount of state relating t

 ### Adding an `onClick` handler to a `Draggable` or a *drag handle*

-You are welcome to add your own `onClick` handler to a `Draggable` or a *drag handle* (which might be the same element). `onClick` events handlers will only be called if we do not block the click. We block click events from occurring when the user was dragging an item. See [#sloppy-clicks-and-click-blocking-](sloppy clicks and click blocking) for more information.
+You are welcome to add your own `onClick` handler to a `Draggable` or a *drag handle* (which might be the same element). `onClick` events handlers will only be called if we do not block the click. We block click events from occurring when the user was dragging an item. See [#sloppy-clicks-and-click-prevention-](sloppy clicks and click prevention) for more information.


Is this line still true? Won't onClick event handlers now be called but with e.defaultPrevented set to true?

Blasz · 2018-03-12T03:00:07Z

src/debug/timings.js

+const flag: string = '__react-beautiful-dnd-debug-hook__';
+
+// temp
+// window[flag] = true;


Should this be left in?

I'll clean it up

Blasz · 2018-03-12T03:07:01Z

src/view/drag-handle/drag-handle-types.js

@@ -36,8 +36,7 @@ export type DragHandleProps = {|

  // Stop html5 drag and drop
  draggable: boolean,
-  onDragStart: () => boolean,
-  onDrop: () => boolean


Should all references to onDrop be removed or do they refer to different things? Namely the onDrop prop in the Callbacks type above and its usages.

Blasz · 2018-03-12T03:18:06Z

src/view/drag-handle/sensor/create-keyboard-sensor.js

    }
+
+    preventStandardKeyEvents(event);


Would it be worthwhile adding the keycodes that we're specifically handling above into this function as well so that we don't need to add event.preventDefault inside each conditional?

these keys are ones that are prevented by standard across multiple sensors

the keyboard sensor has its own unique keyboard controls

Blasz · 2018-03-12T03:22:26Z

src/view/drag-handle/sensor/create-mouse-sensor.js

@@ -41,7 +46,8 @@ export default ({
  const isDragging = (): boolean => state.isDragging;
  const isCapturing = (): boolean => Boolean(state.pending || state.isDragging);
  const schedule = createScheduler(callbacks);
-  const clickBlocker: ClickBlocker = createClickBlocker();
+  const getWindow = (): HTMLElement => getWindowFromRef(getDraggableRef());
+  const postDragEventPreveter: EventPreventer = createPostDragEventPreventer(getWindow);


Variable typo

Blasz · 2018-03-12T03:36:01Z

test/unit/view/drag-handle.spec.js

+        describe('cancelled with any keydown', () => {
+          Object.keys(keyCodes).forEach((key: string) => {
+            describe(`with the ${key} key`, () => {
+              it('should not call execute any callbacks', () => {


Typo I think, one of call/execute should be removed?

Blasz · 2018-03-12T03:40:03Z

test/unit/view/drag-handle.spec.js

@@ -820,9 +958,39 @@ describe('drag handle', () => {
        })).toBe(true);
      });

-      it('should not do anything if there is nothing dragging', () => {
+      it('should block a click after a cancel', () => {


block -> prevent?

yep. i have also updated the other places that used the blocked language

Blasz · 2018-03-12T03:44:13Z

test/unit/view/drag-handle.spec.js

@@ -1,5 +1,6 @@
 // @flow


This is a big file!

yep. needs to be split up

alexreardon added 14 commits March 9, 2018 13:24

initial

f48f46a

progress

be3f1ed

nested

7db67d2

cleanup

ba3f463

more love for drag handle tests

059fb5a

renaming

8bdf16a

refinements

186ab0a

updating docs

c381a11

fixing tests

2dda6a8

improving clarity

3702d1b

simplier event marshal

6e167b0

simplifying event handling - no more timeouts

482f036

more cleanup

f60cf4f

fixing linting and flow

de6826a

alexreardon commented Mar 12, 2018

View reviewed changes

alexreardon added 2 commits March 12, 2018 11:33

removing old comments

9e40749

Merge branch 'master' of github.com:atlassian/react-beautiful-dnd int…

b4f355b

…o event-handling

Blasz approved these changes Mar 12, 2018

View reviewed changes

alexreardon added 3 commits March 12, 2018 15:57

fixing tests

b46b4f2

fixing language

e31af42

updating types in README

8731c65

alexreardon merged commit e8da50f into master Mar 12, 2018

alexreardon deleted the event-handling branch March 12, 2018 05:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved event handling #379

Improved event handling #379

alexreardon commented Mar 9, 2018

alexreardon Mar 11, 2018

alexreardon Mar 11, 2018

alexreardon Mar 11, 2018

alexreardon Mar 11, 2018

alexreardon Mar 11, 2018

alexreardon Mar 11, 2018

Blasz Mar 12, 2018 •

edited

Loading

alexreardon Mar 11, 2018

alexreardon Mar 11, 2018

alexreardon Mar 11, 2018

alexreardon Mar 12, 2018

Blasz left a comment

Blasz Mar 12, 2018

Blasz Mar 12, 2018

alexreardon Mar 12, 2018

Blasz Mar 12, 2018

alexreardon Mar 12, 2018

Blasz Mar 12, 2018 •

edited

Loading

Blasz Mar 12, 2018

alexreardon Mar 12, 2018

alexreardon Mar 12, 2018

Blasz Mar 12, 2018

alexreardon Mar 12, 2018

Blasz Mar 12, 2018

alexreardon Mar 12, 2018

Blasz Mar 12, 2018

alexreardon Mar 12, 2018

Blasz Mar 12, 2018

alexreardon Mar 12, 2018

		configure(loadStories, module);

		// console.log('Adding some window event handlers to the story');

Improved event handling #379

Improved event handling #379

Conversation

alexreardon commented Mar 9, 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

Blasz Mar 12, 2018 • edited Loading

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

Blasz left a 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

Choose a reason for hiding this comment

Blasz Mar 12, 2018 • edited Loading

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

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

Blasz Mar 12, 2018 •

edited

Loading

Blasz Mar 12, 2018 •

edited

Loading