Drag Meta Provider #637

kitsonk · 2017-08-17T15:48:28Z

Type: Feature

The following has been addressed in the PR:

There is a related issue
All code matches the style guide
Unit or Functional tests are included in the PR

Description:

This is a proof of concept meta provider which provides drag information to a widget instance. The provider provides a DragResults every time it is called. Those drag results provide if the node is being currently dragged isDragging and any change in position from the last time the provider was called.

An example of how this would look in a real world implementation would be something like this:

export interface ScrollBarProperties extends WidgetProperties {
  horizontal: boolean;
  position: number;
  size?: number;
  sliderMin?: number;
  sliderSize?: number;
  visible?: boolean;
  onScroll?(delta: number): void;
}

class ScrollBar extends WidgetBase<ScrollBarProperties> {
  const { key, horizontal } = this.properties;
  let isDragging = false;
  if (onScroll) {
    const dragResult = this.meta(Drag).get('slider');
    const delta = horizontal ? dragResult.delta.x : dragResult.delta.y;
    isDragging = dragResult.isDragging;
    onScroll(delta);
  }

  // some calculated styles to position the slider
  // the UI can also render different if the slider is being dragged
  return v('div', { key }, [ v('div', { key: 'slider' }) ]);
}

Because the DragController tracks the events from the window level, but is able to attribute it back to a particular node, tracking of the events keeps the drag state accurate. Here is an example of it in action:

Fixes: #571 (in part)

kitsonk · 2017-09-07T13:55:28Z

We should support pointer events as part of this provider and it should not need to deal with touch/mouse events then.

agubler · 2017-09-20T19:36:01Z

src/meta/Drag.ts

+	/**
+	 * The movement of pointer during the duration of the drag state
+	 */
+	delta: Position;


nit: should we declare the interface before we use it? (I thought it was an from an import)

I was favouring alphabetisation over use before declaration... one form of clarity causes another issue

I guess so.

agubler · 2017-09-20T19:36:54Z

src/meta/Drag.ts

+
+/**
+ * Return the delta position between two positions
+ * @param start The first posistion


posistion -> position

agubler · 2017-09-20T19:38:03Z

src/meta/Drag.ts

+
+	private _getData(target: HTMLElement): { state: NodeData, target: HTMLElement } | undefined {
+		if (this._nodeMap.has(target)) {
+			return { state: this._nodeMap.get(target)!, target };


it would be so good if TS could know that we are in a block narrowed by this._nodeMap.has(target)

See: microsoft/TypeScript#11117

Not surprised there’s an issue. That would be nice.

agubler · 2017-09-20T19:39:43Z

src/meta/Drag.ts

+		}
+	}
+
+	private _onDragStart = (e: PointerEvent) => {


any reason why private _onDragStart = (e: PointerEvent) => { over private _onDragStart(e: PointerEvent) {?

nit: could we have event over e?

Why the lambda, is because the handler needs to be bound to the instance. When used as a handler, the this scope is the window I believe.

Gotcha re scope. Over having a separate variable that binds scope to the method (we have used both patterns)

Yeah... I like this better for class methods where it effects the code readability less, IMO. Using the bound variable makes a bit of indirection when reading the code.

Agreed, for private methods.

agubler · 2017-09-20T19:41:07Z

src/meta/Drag.ts

+	}
+
+	constructor() {
+		const win: Window = global.window;


Don't want to mask the global. We have tended to now do that, using doc or win when using a reference to a potential global.

agubler · 2017-09-20T19:41:58Z

src/meta/Drag.ts

+		// first time we see a node, we will initialize its state
+		if (!_nodeMap.has(node)) {
+			_nodeMap.set(node, {
+				dragResults: {


can you use emptyResults?

apart from it being frozen... could we just deep assign the emptyResults when we return them?

well, empty results is valid here, I will use that

agubler · 2017-09-20T19:42:21Z

src/meta/Drag.ts

+
+const controller = new DragController();
+
+export default class Drag extends Base {


generally we do a named export and a default export across widget-core.

agubler · 2017-09-20T19:42:49Z

src/meta/Drag.ts

+const controller = new DragController();
+
+export default class Drag extends Base {
+	private boundInvalidate = this.invalidate.bind(this);


_boundInvalidate

agubler · 2017-09-20T19:44:11Z

src/meta/Drag.ts

+
+/**
+ * Return the x/y position for an event
+ * @param e The MouseEvent or TouchEvent


now the PointerEvent?

pottedmeat

Minor stuff

pottedmeat · 2017-10-02T20:05:27Z

src/meta/Drag.ts

+const controller = new DragController();
+
+export class Drag extends Base {
+	private _boundInvalidate: () => void = this.invalidate.bind(this);


This should already be a bound version that comes from WidgetBase

It is its own .invalidate which isn't auto bound I believe... because this doesn't come from WidgetBase. I will double check though.

While the properties passed in at creation provides a bound invalidate, that is a private property on base and the public invalidate() calls that private property that is copied out, therefore when called from event handlers, the Base.invalidate() loses its context and needs to be statically bound. This is likely undesirable, and we should consider ensuring that Base.invalidate() will always call the right invalidate, but for now, this needs to stay this way to work. I will open an issue to consider the change to Base.

pottedmeat · 2017-10-02T20:06:56Z

src/meta/Drag.ts

+
+class DragController {
+	private _nodeMap = new WeakMap<HTMLElement, NodeData>();
+	private _dragging: HTMLElement | undefined = undefined;


Couldn't this be private _dragging?: HTMLElement;

It could be, but considering I was explicitly assigning undefined, the property will always be on the instance. Felt more accurate to not be optional.

pottedmeat · 2017-10-02T20:39:05Z

tests/unit/meta/Drag.ts

+						key: 'child1'
+					}),
+					v('div', {
+						innerHTML: 'Hello Wolrd',


Not that it matters, but a typo :P

pottedmeat · 2017-10-02T20:42:30Z

tests/unit/meta/Drag.ts

+		document.body.removeChild(div);
+	},
+
+	'non draggable node'() {


It might be worth naming this differently. I was thinking non-draggable meant something marked as non-draggable by CSS or attributes and got confused - but this is testing that a node where drag events haven't been requested doesn't generate drag events.

Rename it to?

Non listening node?

kitsonk changed the title ~~POC: Drag Meta Provider~~ Drag Meta Provider Aug 22, 2017

pottedmeat mentioned this pull request Aug 23, 2017

Integrate Drag Meta for Virtual Scrollbar dojo/dgrid#47

Open

dylans added this to the 2017.09 milestone Sep 7, 2017

kitsonk added 6 commits September 20, 2017 13:06

WIP: Drag meta

e680af1

Fixup for POC Drag

2dce035

Fix test name

ca2c502

Flesh out test cases

f0e41b9

Support dragging on child nodes

ba7afc7

Use PointerEvents

ddc825d

kitsonk force-pushed the feature-drag-meta branch from 7db2806 to ddc825d Compare September 20, 2017 12:28

agubler reviewed Sep 20, 2017

View reviewed changes

kitsonk added 2 commits September 23, 2017 11:04

Updates based on feedback

5d637fe

Merge remote-tracking branch 'upstream/master' into feature-drag-meta

6538b05

pottedmeat approved these changes Oct 2, 2017

View reviewed changes

agubler approved these changes Oct 6, 2017

View reviewed changes

kitsonk added 2 commits October 6, 2017 13:22

Merge remote-tracking branch 'upstream/master' into feature-drag-meta

9a318b7

Fixes based on feedback

9ba4fae

kitsonk mentioned this pull request Oct 6, 2017

Meta Base invalidate() should not require instance context #708

Open

kitsonk merged commit 8fb3882 into dojo:master Oct 6, 2017

kitsonk modified the milestones: 2017.09, 2017.10 Oct 6, 2017

kitsonk added the beta3 label Oct 6, 2017


		const controller = new DragController();

		export default class Drag extends Base {

Drag Meta Provider #637

Drag Meta Provider #637

Conversation

kitsonk commented Aug 17, 2017 • edited Loading

kitsonk commented Sep 7, 2017

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

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

pottedmeat 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

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

kitsonk commented Aug 17, 2017 •

edited

Loading