Modernize polaris-color-picker components #493

tomnez · 2020-02-05T17:03:09Z

Updates PolarisColorPicker to tagless component, ES6 classes, and angle bracket syntax.

A lot of changes are just codemod changes - additional edits to the component worth pointing out are:

Use of {{did-insert}} and {{will-destroy}} modifiers so we can set a unique ID on the tagless component since the component files made use of this.element.
Added a get element() computed property to return the container element, if we think this will be confusing we could rename to something specific per component, i.e. get alphaPickerElement()
Added template-lint-disable no-invalid-interactive to the slider because it makes use of onmousedown and ontouchstart events.

Dummy app demo:

vladucu

@tomnez seems some tests are failing....can you please re-tag me once they pass

andrewpye

Generally looking good @tomnez, but for whatever reason there's an iffy error on CI 😭

andrewpye · 2020-02-06T07:21:12Z

addon/components/polaris-color-picker.js

+  }
+
+  @computed('contentId')
+  get element() {


Curious what @vladucu's thoughts are about this naming, mostly because I'm conflicted: part of me feels like we shouldn't be overwriting Ember's internal API, part of me feels like we should be able to overwrite because that's why we do inheritance. Basically I'm good with this as is but would also understand if we'd prefer to name it something else 😝

hmm, tbh...I'm not sure I understand why we need this

@vladucu you're confused as to why it's a CP, or confused as to why we need to set the element?

I guess we could just set it in the init hook since that's the only place it's used. I was just trying to keep the same API that we were used to (having the use of this.element) but I can get rid of it. If we ever end up needing it in multiple methods then we can refactor to a CP again.

yeah, why we need the CP....we get the element explicitly as first parameter in the action called through did-insert modifier....we could just set it here as a property on the component....but unless I'm wrong, it's only used in this one place

update - was going through other files, and components like polaris-color-picker/slidable call this.element from multiple places, so maybe it's easiest to have it as a CP in this particular case?

If not we can call document.querySelector('#${this.contentId}') each time. Pretty much the same thing IMO 🤷‍♂

note: maybe I'm missing something and just talking non-sense...feel free to correct me
what I mean is....we can just keep it simple....why add a CP when you already have an action that set's contentId and then have the CP compute what you call element now

even more...could this just be? Not sure where the content stuff comes from in this PR....

/** * Polaris color picker component. * See https://polaris.shopify.com/components/forms/color-picker */ @tagName('') @templateLayout(layout) export default class PolarisColorPicker extends Component { .... // NO LONGER NEEDED // didRender() { // super.didRender(...arguments); // let element = this.element; // if (isNone(element)) { // return; // } // Grab the size of the picker for positioning the draggable markers. // const mainColorElement = element.querySelector( // 'div.Polaris-ColorPicker__MainColor' // ); // if (isNone(mainColorElement)) { // return; // } // this.set('pickerSize', mainColorElement.clientWidth); // } @action setPickerSize(element) { // Grab the size of the picker for positioning the draggable markers. const mainColorElement = element.querySelector( 'div.Polaris-ColorPicker__MainColor' ); this.set('pickerSize', mainColorElement.clientWidth); } ... }

<div id={{this.contentId}} class="Polaris-ColorPicker" {{did-insert this.setContentId}} > <div class="Polaris-ColorPicker__MainColor"> ... </div> .... </div>

^ Agreed, this works for polaris-color-picker, but other components will have multiple methods that need to access the container element, so in other components we should at least set element as a property on did-insert (see slidable.js, it accessing element in other places besides the did-insert method).

So I figure I'll refactor where I can, but some components might still have the element property set.

don't get me wrong Tom....in the same action you call from did-insert, of course, keep a reference to the mainColorElement or whatever you need by setting it on the component....if you need that for later use too

andrewpye · 2020-02-06T07:22:30Z

addon/components/polaris-color-picker.js

+  }
+
+  @action
+  setContentId(element, [value]) {


Reckon this, then contentId property etc. are worth putting into a mixin? We're already using them 4 times in this PR alone 🤔

we should probably stop using mixins going further, if we want to be able to use Glimmer components
what do you think about looking later into drying this up?

addon/components/polaris-color-picker.js

vladucu

Looking good so far...left a few comments though

vladucu · 2020-02-06T12:58:56Z

addon/components/polaris-color-picker.js

+  }
+
+  @action
+  setContentId(element, [value]) {


we should probably stop using mixins going further, if we want to be able to use Glimmer components
what do you think about looking later into drying this up?

addon/components/polaris-color-picker.js

vladucu · 2020-02-06T13:10:16Z

addon/components/polaris-color-picker.js

+  }
+
+  @computed('contentId')
+  get element() {


hmm, tbh...I'm not sure I understand why we need this

vladucu · 2020-02-06T13:12:21Z

addon/components/polaris-color-picker.js


  didRender() {
-    this._super(...arguments);
+    super.didRender(...arguments);


I don't think we need didRender anymore...actually we want to stop using any other lifecycle hooks except init/destroy....instead we should have this logic inside the action you added setContentId (renamed ofc)

vladucu · 2020-02-06T13:13:17Z

addon/components/polaris-color-picker.js

+      pickerSize,
+      color: { hue, alpha = 1 },
+      onChange,
+    } = this.getProperties('pickerSize', 'color', 'onChange');


Suggested change

} = this.getProperties('pickerSize', 'color', 'onChange');

} = this;

vladucu · 2020-02-06T13:14:03Z

addon/components/polaris-color-picker.js

+    const {
+      color: { brightness, saturation, alpha = 1 },
+      onChange,
+    } = this.getProperties('color', 'onChange');


Suggested change

} = this.getProperties('color', 'onChange');

} = this;

vladucu · 2020-02-06T13:14:18Z

addon/components/polaris-color-picker.js

+    const {
+      color: { hue, brightness, saturation },
+      onChange,
+    } = this.getProperties('color', 'onChange');


Suggested change

} = this.getProperties('color', 'onChange');

} = this;

vladucu · 2020-02-06T13:14:55Z

addon/components/polaris-color-picker/alpha-picker.js

@@ -1,6 +1,8 @@
+import { tagName, layout as templateLayout } from '@ember-decorators/component';


same here (and other files), can we order this please

vladucu · 2020-02-06T13:15:32Z

addon/components/polaris-color-picker/alpha-picker.js

+  }
+
+  @computed('contentId')
+  get element() {


similar here like in main component

vladucu · 2020-02-06T13:15:44Z

addon/components/polaris-color-picker/alpha-picker.js

+
+  @action
+  handleChange({ y }) {
+    const { sliderHeight, onChange } = this.getProperties(


let's use ES6 destructuring

tomnez · 2020-02-06T20:49:20Z

@vladucu @andrewpye updated - also, I ran the base branch locally and ember-beta and ember-canary are having issues there as well. What are your thoughts on just removing those from our ember-try config while we get the rest of these components updated?

Seems like it would allow us to move a lot faster rather than finding failing tests and skipping them.

vladucu · 2020-02-06T20:53:57Z

addon/components/polaris-color-picker/slidable.js

+  }
+
+  @action
+  setContentId(element, value) {


is this copied from somewhere? I don't understand where it's coming from, can't see it in the version prior to these changes

It's slighty different than the old version, it used to be

setContentId(element, [value])

because it used to get called from the on-insert modifier, but now it gets called from within setContentIdAndTriggerHeightChanged below.

would you mind linking me to the old version here on github?

It's here in my first commit - this link doesn't seem to scroll to the line changes, but its slidable.js, lines 150-153 - 0c66046#diff-160b5cb0d5fd7505911f238ca1295ee4R150-R153

kk....do you think this is needed? why are we adding an ID attirbute to the new wrapping div in the template that was added after making this component tag-less?

this should be much simpler....please check ember-render-modifiers to understand it's usage....it's super simple: did-insert will invoke your action with one param element (the element on which did-insert is added) and whatever else you're passing to it.....just set this element argument you receive in your action and set it on this component

there's no need guidFor and appending content....etc (which is why I was confused)

Ahh I did that originally, but got the error

Cannot set property element of [object Object] which has only a getter

I thought maybe it was something weird with the modifiers but it turns out it just doesn't want me using the key name "element".. 🤦‍♂ So yeah I can just do that.

welp, element is a reserved word in ember so makes sense

Yeah, strange that it let me overwrite it with a CP tho.

Yeah, strange that it let me overwrite it with a CP tho.

I guess overwriting replaces the entire setter/getter combo, while setting tries to access a non-existent setter on the existing element property 🤷‍♂

vladucu

Not sure if you've missed to push the commit, but I still see in slidable things we discussed yesterday to change...can you please take a look and re-tag once ready?

tomnez · 2020-02-07T14:55:21Z

Ahhh yeah forgot to push 🤦‍♂

sivakumar-kailasam · 2020-02-07T15:30:57Z

classic tom 😄

tomnez · 2020-02-07T15:33:30Z

Rough week

vladucu

Thanks for the updates, Tom!
👍

…icker

vladucu · 2020-02-10T10:39:02Z

addon/templates/components/polaris-color-picker.hbs

-</div>
+<div
+  class="Polaris-ColorPicker"
+  {{did-insert this.setPickerSize}}


nit: let's add ...attributes here too

Modernize polaris-color-picker components

0c66046

tomnez requested review from vladucu and andrewpye February 5, 2020 17:03

tomnez self-assigned this Feb 5, 2020

Disable no-invalid-interactive for slidable

3d07abe

vladucu requested changes Feb 5, 2020

View reviewed changes

tomnez force-pushed the refactor/es6-polaris-color-picker branch 2 times, most recently from 7330824 to f7fa53d Compare February 5, 2020 20:16

Replace fn with action helper for mut method

1c58f5f

tomnez force-pushed the refactor/es6-polaris-color-picker branch from f7fa53d to 1c58f5f Compare February 5, 2020 21:06

andrewpye reviewed Feb 6, 2020

View reviewed changes

vladucu requested changes Feb 6, 2020

View reviewed changes

tomnez added 2 commits February 6, 2020 12:20

PR edits, remove unneeded CP

16ac058

Lint all the things

000f910

tomnez requested review from andrewpye and vladucu February 6, 2020 20:47

vladucu reviewed Feb 6, 2020

View reviewed changes

Set element on did-insert

3961196

tomnez requested a review from vladucu February 6, 2020 21:33

vladucu requested changes Feb 7, 2020

View reviewed changes

tomnez requested a review from vladucu February 7, 2020 14:55

vladucu approved these changes Feb 7, 2020

View reviewed changes

Merge branch 'refactor/es6-classes' into refactor/es6-polaris-color-p…

5dd0730

…icker

vladucu reviewed Feb 10, 2020

View reviewed changes

Add attributes to template

8e41386

tomnez merged commit db1b6f9 into refactor/es6-classes Feb 10, 2020

tomnez deleted the refactor/es6-polaris-color-picker branch February 10, 2020 17:28

vladucu added the internal label Jun 26, 2020

	} = this.getProperties('pickerSize', 'color', 'onChange');
	} = this;

		@@ -1,6 +1,8 @@
		import { tagName, layout as templateLayout } from '@ember-decorators/component';

Modernize polaris-color-picker components #493

Modernize polaris-color-picker components #493

Conversation

tomnez commented Feb 5, 2020 • edited

Dummy app demo:

vladucu left a comment

Choose a reason for hiding this comment

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

tomnez Feb 6, 2020 • edited

Choose a reason for hiding this comment

vladucu Feb 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladucu 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

tomnez commented Feb 6, 2020

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

vladucu left a comment

Choose a reason for hiding this comment

tomnez commented Feb 7, 2020

sivakumar-kailasam commented Feb 7, 2020

tomnez commented Feb 7, 2020

vladucu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomnez commented Feb 5, 2020 •

edited

tomnez Feb 6, 2020 •

edited

vladucu Feb 6, 2020 •

edited