Code review for two-up, pinch-zoom, trackDragging

[jakearchibald]

You can npm i && npm run start in the components folder to build the test page.

See comments for which files to ignore.

[jakearchibald]

Only for the demo stuff. This will be removed in the final merge.

[jakearchibald]

This is a stub I created for @kosamari. Feel free to ignore.

[jakearchibald]

This is a stub I created for @kosamari. Feel free to ignore.

[jakearchibald]

This is just so I can build the test page. Will be removed in the final merge in favour of the build system @developit is working on.

[jakearchibald]

Just for the test page.

[jakearchibald]

Just for the test page.

[jakearchibald]

Just for the test page.

[jakearchibald]

Just for the test page.

[jakearchibald]

Although we'll need the event retargeting stuff.

[jakearchibald]

Just for the test page.

[jakearchibald]

This will be merged with the overall project rules.

[jakearchibald]

This will be merged with the overall project rules.

[jakearchibald]

This will be dropped in favour of @developit's build system.

[surma]

Just a couple of minor things.

I’ll have to give trackDragging.ts another review when my mind works properly 😄

[surma]

Any reason why b is optional?

[jakearchibald]

Convenience really, so the distance is zero when there's only one pointer.

[surma]

But can that ever happen? Wouldn’t we rather throw a compile-time error when you call this function with just one argument?

[jakearchibald]

It happens a lot – if you're dragging using the mouse, or with one finger on the screen.

Alternatively I could throw an if around every usage of getDistance and getMidpoint, but that feels messier.

[surma]

Fair.

[surma]

I find this notation quite hard to read. WDYT about defining an setTransformOptions type right above?

[surma]

Maybe even this?

interface setTransformOptions {
    allowChangeEvent?: boolean;
}
const setTransformDefaultOpts: setTransformOptions = {
    allowChangeEvent: false;
};
function setTransform(scale: number, x: number, y: number, opts: setTransformOptions?) {
    opts = { ...setTransformDefaultOpts, ...opts };
    /* ... */
}```

[surma]

Or we even go full TypeScript:

const setTransformDefaultOpts = {
    allowChangeEvent: false;
};
function setTransform(scale: number, x: number, y: number, opts: Partial<typeof setTransformDefaultOpts>?) {
    opts = { ...setTransformDefaultOpts, ...opts };
    /* ... */
}

[jakearchibald]

After in person discussion, we decided:

BAD JakeScript

private _applyChange ({
  panX = 0,
  panY = 0,
  scaleDiff = 1,
  originX = 0,
  originY = 0
}: {
  panX?: number,
  panY?: number,
  scaleDiff?: number,
  originX?: number,
  originY?: number
} = {}) {
  // …
}

HalloumiScript

interface ApplyChangeOpts {
  panX?: number;
  panY?: number;
  scaleDiff?: number;
  originX?: number;
  originY?: number;
}

private _applyChange (opts: ApplyChangeOpts = {}) {
  const {
    panX = 0, panY = 0,
    originX = 0, originY = 0,
    scaleDiff = 1
  } = opts;
  // …
}

Any objections / better ideas @developit @kosamari?

[surma]

Nit: fireChangeEvent?

[jakearchibald]

I thought the same, but doesn't fireChangeEvent make it sound like it will fire a change event, whereas in reality it might not, if the provided values match the current state, or are clamped to be equal to the current state?

[surma]

I think the word “change” covers that.

[jakearchibald]

@developit @kosamari what do you think?

pinchZoom.setTransform(1, 100, 100, {
  allowChangeEvent: true
});

Or fireChangeEvent?

If you set this property to true, it may not fire a change event, eg if the supplied or computed values are the same as the current transform.

[surma]

What’s our stance on re-using platform events? Should CEs always fire CustomEvent?

[jakearchibald]

Asked in web platform kin.

[jakearchibald]

Seems like the only reason to use CustomEvent is for .detail, which we don't need here (and wouldn't want to use in TypeScript).

[surma]

Okay, crazy idea: this.children.length === 1 && this.children[0] instanceof HTMLElement

[jakearchibald]

I don't know why _positioningEl needs to be an HTMLElement. I complained at one point, but I've refactored since, so it shouldn't be an issue.

[surma]

Is the second :not() necessary considering the code makes sure that the handle is always the last child?

[jakearchibald]

If there are no children other than the handle it'll be both the last and first child.

[surma]

Same here.

[surma]

@bind?

[surma]

Maybe define an enum for LEFT_BUTTON?

[surma]

Can you use isPointerEvent here?

[surma]

Same here

[surma]

@Bind?

[developit]

Maybe amend when I merge - I exported it from src/lib/util.ts

[surma]

Maybe define an enum for LEFT_BUTTON?

[jakearchibald]

TIL const enum in TypeScript.

I'd avoided enums previously because of the objects they create.

[surma]

Can you use isPointerEvent here?

[jakearchibald]

This is checking for a touch event, not a pointer event.

I could replace this with: self.TouchEvent && event instanceof TouchEvent, but I went for the property check since it's smaller.

[jakearchibald]

I still need to take a look at setTransform in pinch-zoom, but my brain's tired. I botched a refactor. I need to ensure that transforms can be set before the element is connected, and that bounds are checked when setTransform is called.

Also, bounds should be checked on connect.

[jakearchibald]

Thanks for the review @surma! Good to see some conventions for the rest of the project coming out of this.

[jakearchibald]

I've fixed the pinch-zoom stuff. setTransform is now the main method for changing transforms & performs all the bounds checks.

[surma]

Why not put this in a isTouchPointer() like isPointerEvent()?

[jakearchibald]

It's only used once. Happy to put it in a function if you'd prefer though.

[surma]

re-use isTouchEvent?

[jakearchibald]

There's no such method

[surma]

Okay, since we deferred @bind, I am okay with finally going ahead with this (sorry!)

[jakearchibald]

Closing this, as I'm rolling the components into master.