correctly blocking onClick for all element types #353

alexreardon · 2018-02-27T03:56:59Z

Fixes #273.

This is required for #10.

onClick event handlers are not firing on the drag handle because it has pointer-events: none; applied to it. This means that it fires on the parent. Confusingly the onClick handler will fire on the element if the element is an anchor. There is a work around (see #10) but it is hacky. This change would make onClick fire correctly on the drag handle every time. It involves removing pointer-events: none off the dragging drag handle. This has no performance implications.

There is a drawback. By changing this you will no longer be able to use the mouse wheel to scroll a Droppable underneath a dragging item. Disabling pointer events allowed the user to 'scroll through' the dragging item. Turning them back on means that they can no longer do this. However, I think given that users can now automatically scroll the window and scroll containers by using mouse movement this is acceptable. I have investigated the other drag and drop libraries out there that I could and they all have this restriction.

I think on the whole this is a positive and useful change as it delivers more thoroughly on our attempt to correctly block click events

…o onclick-publish-fix

jaredcrowe · 2018-02-27T05:37:04Z

src/view/style-marshal/get-styles.js

@@ -113,8 +114,8 @@ export default (styleContext: string): Styles => {
  // > Applied while the user is actively dragging

  // cursor: grab
-  // We apply this by default for an improved user experience. It is such a common default that we
-  // bake it right in. Consumers can opt out of this by adding a selector with higher specificity
+  // We apply apply this style to the body in case the user pointer


absolute nitpick but you've written 'apply' here twice

i'll fix it

alexreardon · 2018-02-27T06:41:45Z

It sucks we need to do this - but it is for the best

alexreardon · 2018-02-28T00:14:48Z

I was not happy to loose wheel scrolling so I have changed how we do post drag click prevention. It is working really well.

alexreardon · 2018-02-28T00:16:54Z

A point for discussion is whether this should be a breaking change. I have currently removed onClick from DragHandleProps which is a breaking change. I add it back in and make it a no-opt to make this a non breaking change. However, doing this means that future consumers need to monkey patch onClick for no reason until we do the next major release (which sucks). Also, we are techinically changing the behaviour of the onClick function. Before if it was an anchor it would be called every time regardless of whether we performed click blocking or not. Now it will not be called at all if we are blocking the click. I think this change of behaviour gives further weight to the idea that this should be labeled as a breaking change.

Thoughts @jaredcrowe @Blasz ?

My personal thought is to keep the api as tight as possible and drop onClick and move to 6.0.

alexreardon · 2018-02-28T00:17:31Z

README.md

@@ -366,6 +366,12 @@ point-events: none;

 **Styles applied using the `data-react-beautiful-dnd-draggable` attribute**

+We apply a cursor while dragging to give user feedback that a drag is occurring. You are welcome to override this by applying your own cursor to the element with a greater specificity.


I need to remove these new comments (they are no longer needed)

alexreardon · 2018-02-28T00:17:46Z

README.md

@@ -1142,23 +1146,20 @@ Controlling a whole draggable by just a part of it
 You can override some of the `dragHandleProps` props with your own behavior if you need to.

 ```js
-const myOnClick = event => console.log('clicked on', event.target);
+const myOnMouseDown = event => console.log('mouse down on', event.target);


no longer using 'click' as part of the example

alexreardon · 2018-02-28T00:18:21Z

src/view/style-marshal/get-styles.js

@@ -65,9 +64,11 @@ export default (styleContext: string): Styles => {
        cursor: grab;
      }
    `,
-    blockPointerEvents: `
+    whileActiveDragging: `


This needs to be undone

alexreardon · 2018-02-28T00:37:07Z

I have tested this on our supported desktop browsers. I still need to test it on mobile (ios and andriod)

alexreardon · 2018-02-28T00:39:27Z

It is looking good on ios. I'll check andriod after lunch

jaredcrowe · 2018-02-28T00:51:24Z

If it wasn't for the fact that anchors won't always get their onClick called now I would have said make DragHandleProps.onClick a no-op and remove it next major. But since that behaviour is changing I agree that we should probably make a breaking change.

correctly blocking onClick for all element types

e739271

alexreardon requested a review from jaredcrowe February 27, 2018 03:57

alexreardon added 3 commits February 27, 2018 15:03

adding comments

43cca2e

updating naming and readme

592c7e1

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

b84b1ce

…o onclick-publish-fix

jaredcrowe reviewed Feb 27, 2018

View reviewed changes

jaredcrowe approved these changes Feb 27, 2018

View reviewed changes

alexreardon added 2 commits February 27, 2018 16:56

improving readme

34eb41b

fixing comment

99fcaab

jaredcrowe approved these changes Feb 27, 2018

View reviewed changes

alexreardon added 6 commits February 27, 2018 20:25

mouse wheel scrolling is back

8cded9b

tightening up structure

1301969

simplifying and adding mobile support

b130396

cleanup of types

71c6e70

updating tests

b8ebf3e

updating readme

7cb55fe

alexreardon commented Feb 28, 2018

View reviewed changes

alexreardon added 2 commits February 28, 2018 11:25

readme fix

f288e54

reverting get style changes

4dbdd94

jaredcrowe approved these changes Feb 28, 2018

View reviewed changes

alexreardon added 2 commits February 28, 2018 14:18

moving content around

342501b

correcting DragHandleProps type in readme

fcf4e5d

alexreardon merged commit 566fc69 into master Feb 28, 2018

alexreardon deleted the onclick-publish-fix branch February 28, 2018 03:28

This was referenced Feb 28, 2018

Community avatar drag strangeness element-hq/element-web#6156

Closed

Dropping an icon in the LLP while filtering causes the filter to be cleared element-hq/element-web#6253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

correctly blocking onClick for all element types #353

correctly blocking onClick for all element types #353

alexreardon commented Feb 27, 2018

jaredcrowe Feb 27, 2018

alexreardon Feb 27, 2018

alexreardon Feb 27, 2018

alexreardon commented Feb 27, 2018

alexreardon commented Feb 28, 2018

alexreardon commented Feb 28, 2018 •

edited

Loading

alexreardon Feb 28, 2018

alexreardon Feb 28, 2018

alexreardon Feb 28, 2018

alexreardon commented Feb 28, 2018

alexreardon commented Feb 28, 2018

jaredcrowe commented Feb 28, 2018

		@@ -366,6 +366,12 @@ point-events: none;

		Styles applied using the `data-react-beautiful-dnd-draggable` attribute

		We apply a cursor while dragging to give user feedback that a drag is occurring. You are welcome to override this by applying your own cursor to the element with a greater specificity.

correctly blocking onClick for all element types #353

correctly blocking onClick for all element types #353

Conversation

alexreardon commented Feb 27, 2018

jaredcrowe Feb 27, 2018

Choose a reason for hiding this comment

alexreardon Feb 27, 2018

Choose a reason for hiding this comment

alexreardon Feb 27, 2018

Choose a reason for hiding this comment

alexreardon commented Feb 27, 2018

alexreardon commented Feb 28, 2018

alexreardon commented Feb 28, 2018 • edited Loading

alexreardon Feb 28, 2018

Choose a reason for hiding this comment

alexreardon Feb 28, 2018

Choose a reason for hiding this comment

alexreardon Feb 28, 2018

Choose a reason for hiding this comment

alexreardon commented Feb 28, 2018

alexreardon commented Feb 28, 2018

jaredcrowe commented Feb 28, 2018

alexreardon commented Feb 28, 2018 •

edited

Loading