Upgrade dropzone to v2.11.0 #225

tomnez · 2018-10-23T20:14:23Z

Finishes #223

addon/components/empty-wrapper.js

andrewpye

Great job with this @tomnez, thanks for tackling it! I think we can remove a couple of unnecessary new files (empty-wrapper and create-unique-id-factory) to clean things up a bit, other than that just a couple of questions really 👍

andrewpye · 2018-10-24T06:58:11Z

addon/components/polaris-caption.js

@@ -20,4 +20,6 @@ export default Component.extend({
   * @default: null
   */
  text: null,
+
+  'data-test-caption': true,


What do you think of passing data-test-caption=true to the instance of this where you need it, instead of adding this to all instances via the class definition?

agree this is not necessary, but nice to have, ex: a component doesn't have this and another one that uses it is being tested...you can't really add to the instance

Not sure I follow your example @vladucu but I don't feel particularly strongly about this either way

I'd prefer to keep it here, the selectors get removed in a non dev/test environment so I think it's fine to have all instances have these attrs. It would save time in test writing knowing that you can always grab [data-test-<whatever>] rather than have to go see if the template already has the attr or not.

andrewpye · 2018-10-24T07:00:00Z

addon/components/polaris-drop-zone.js

@@ -4,6 +4,7 @@ import { or } from '@ember/object/computed';
 import { classify } from '@ember/string';
 import { throttle, scheduleOnce } from '@ember/runloop';
 import { isNone, isPresent } from '@ember/utils';
+import { createUniqueIDFactory } from '../utils/create-unique-id-factory';


We've eschewed this in the past in favour of using Ember's unique IDs via guidFor(this), is there any reason not to continue that path here?

We can swap for guideFor, I was just following the React version.

Yeah I know, just that so far we've avoided doing that and I'd like to keep our addon self-consistent. We can always look at switching all of the guidFors across to follow the React implementation at a later stage if we feel it's valuable 👍

yeah, I think this is not really a problem, but more an advantage that we use Ember 😉

andrewpye · 2018-10-24T07:01:52Z

addon/components/polaris-drop-zone.js

+   * @public
+   * @property id
+   */
+  id: null,


Not 100% sure how Ember handles passing id=<whatever> into a component, but from a faint memory I think it might use that as the id of that component's element, which will cause problems if we then try using that same ID on another element. With that in mind, we might be safer renaming this to something like inputId (as I think we've done for a bunch of other form-related components) and documenting the difference from the React interface in the component's markdown file.

You could also default this to a CP that returns guidFor(this), which I think we've also done elsewhere, if that cleans up logic later on.

I think it should be fine setting the id as long as it's unique (we don't re-use it in this component on multiple components - which I don't think we do)
setting a default seems to be done in getDerivedStateFromProps(), but agree we have guidFor instead of createUniqueIDFactory

andrewpye · 2018-10-24T07:05:59Z

addon/components/polaris-drop-zone.js

@@ -477,31 +539,40 @@ export default Component.extend(ContextBoundEventListenersMixin, {
      return;
    }

+    let dropzoneContainer = this.element.querySelector('.Polaris-DropZone');


Since we're using this in a couple of places, how do you feel about making a dropzoneContainer CP?

I think we're setting this as this.node (in setupNode()) before this runs, so we should just use that everywhere else?

addon/templates/components/polaris-drop-zone.hbs

andrewpye · 2018-10-24T07:17:15Z

addon/utils/create-unique-id-factory.js

+export function createUniqueIDFactory(prefix) {
+  let index = 1;
+  return () => `${prefix}${index++}`;
+}


Can possibly remove this file because of my other comment about using guidFor

vladucu · 2018-10-24T10:04:48Z

addon/components/polaris-caption.js

@@ -20,4 +20,6 @@ export default Component.extend({
   * @default: null
   */
  text: null,
+
+  'data-test-caption': true,


agree this is not necessary, but nice to have, ex: a component doesn't have this and another one that uses it is being tested...you can't really add to the instance

vladucu · 2018-10-24T10:09:30Z

addon/components/polaris-drop-zone.js

@@ -477,31 +539,40 @@ export default Component.extend(ContextBoundEventListenersMixin, {
      return;
    }

+    let dropzoneContainer = this.element.querySelector('.Polaris-DropZone');


I think we're setting this as this.node (in setupNode()) before this runs, so we should just use that everywhere else?

vladucu · 2018-10-24T10:16:46Z

addon/templates/components/polaris-drop-zone.hbs

+      {{#polaris-visually-hidden}}
+        <input
+          data-test-drop-zone-hidden-input
+          id={{concat elementId "-input"}}


I think we've missed updating here to use the id

also, in JS, we'll want to tweak fileInputNode CP?

vladucu · 2018-10-24T10:19:48Z

addon/components/polaris-drop-zone.js

+   * @public
+   * @property id
+   */
+  id: null,


I think it should be fine setting the id as long as it's unique (we don't re-use it in this component on multiple components - which I don't think we do)
setting a default seems to be done in getDerivedStateFromProps(), but agree we have guidFor instead of createUniqueIDFactory

addon/components/polaris-drop-zone.js

vladucu · 2018-10-24T10:27:30Z

tests/integration/components/polaris-drop-zone-test.js

+          label="my label"
+          labelAction=(hash
+            text="my label action"
+            onAction=labelAction


onAction=(action labelAction)

vladucu · 2018-10-24T10:28:53Z

tests/integration/components/polaris-drop-zone-test.js

+      assert
+        .dom(dropZoneLabelWrapperSelector)
+        .exists('A labelled component is wrapped around the dropzone');
+      assert.notOk(


no harm here, though this is unnecessary....this is basically testing polaris-labelled component not firing the action on render....likely just that it fires on click should be enough

Yeah I was just being super cautious about getting a false positive or anything. I'll remove it, this whole action test is likely not necessary since it should be covered in the labelled tests, but I'll at least keep the second assertion.

sounds good to me!

tomnez · 2018-10-24T14:54:12Z

Updated. Notes from the updates:

Left test-selectors as-is
Left id as id, but used guidFor instead of the ID util

andrewpye

I think this looks good now @tomnez! 👍

vladucu

some conflicts, but 🚀

tomnez requested review from andrewpye and vladucu October 23, 2018 20:15

tomnez self-assigned this Oct 23, 2018

tomnez commented Oct 23, 2018

View reviewed changes

addon/components/empty-wrapper.js Outdated Show resolved Hide resolved

andrewpye requested changes Oct 24, 2018

View reviewed changes

vladucu requested changes Oct 24, 2018

View reviewed changes

tomnez requested review from vladucu and andrewpye October 24, 2018 14:54

andrewpye approved these changes Oct 25, 2018

View reviewed changes

vladucu approved these changes Oct 25, 2018

View reviewed changes

tomnez added 8 commits October 25, 2018 14:48

Upgrade drop-zone to v2.11.0

bb92a1d

Fix failing tests after refactor

8038e8a

Swap polaris class selectors for test selectors

508ad6b

Add test for adding label attrs to dropzone

8d3a480

Lint and prettier fixes

20e0d20

Add CHANGELOG entry for polaris-drop-zone

9d0e4fb

PR edits

57c44d9

Disable lint rule in dropzone template

ff9dc05

tomnez force-pushed the upgrade-v2.11.0-dropzone branch from 0d7aec3 to ff9dc05 Compare October 25, 2018 18:49

tomnez merged commit 9320d6d into dev Oct 25, 2018

delete-merged-branch bot deleted the upgrade-v2.11.0-dropzone branch October 25, 2018 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade dropzone to v2.11.0 #225

Upgrade dropzone to v2.11.0 #225

tomnez commented Oct 23, 2018

andrewpye left a comment

andrewpye Oct 24, 2018

vladucu Oct 24, 2018

andrewpye Oct 24, 2018

tomnez Oct 24, 2018

andrewpye Oct 24, 2018

tomnez Oct 24, 2018

andrewpye Oct 25, 2018 •

edited

vladucu Oct 25, 2018

andrewpye Oct 24, 2018

vladucu Oct 24, 2018

andrewpye Oct 24, 2018

vladucu Oct 24, 2018

andrewpye Oct 24, 2018

vladucu Oct 24, 2018

vladucu Oct 24, 2018

vladucu Oct 24, 2018

vladucu Oct 24, 2018

vladucu Oct 24, 2018

vladucu Oct 24, 2018

vladucu Oct 24, 2018

tomnez Oct 24, 2018

vladucu Oct 24, 2018

tomnez commented Oct 24, 2018

andrewpye left a comment

vladucu left a comment

Upgrade dropzone to v2.11.0 #225

Upgrade dropzone to v2.11.0 #225

Conversation

tomnez commented Oct 23, 2018

andrewpye 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

andrewpye Oct 25, 2018 • edited

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

tomnez commented Oct 24, 2018

andrewpye left a comment

Choose a reason for hiding this comment

vladucu left a comment

Choose a reason for hiding this comment

andrewpye Oct 25, 2018 •

edited