Modernize polaris-resource-list components #502

tomnez · 2020-02-12T21:59:19Z

DEV-78

Updates PolarisResourceList and all sub-components to tagless component, ES6 classes, and angle bracket syntax.

Notable changes:

Removed wrapperId from the main polaris-resource-list component. It was only used as a selector in order to set the listNode attribute (ul.Polaris-ResourceList). This was breaking in tests because the ul wasn't rendered yet when setting the node in the component file, so instead I added a {{did-insert this.insertListNode}} on the ul itself and set the reference directly in there.
Used the (element) template helper in favor of our WrapperElement component wherever I could.

Dummy app:

tomnez · 2020-02-12T22:11:48Z

Working on the test failures, FYI

andrewpye · 2020-02-13T07:26:34Z

@tomnez I'll give this a look over once tests are fixed up 👍

tomnez · 2020-02-13T16:32:39Z

@andrewpye behold the passing tests 👼

tomnez · 2020-02-13T16:38:57Z

addon/components/polaris-resource-list/filter-control/filter-value-selector.js

+    // Using `afterRender` here instead of `didInsertElement` hook.
+    // This component is tagless and has no container div so we can't
+    // use `{{did-insert}}` modifiers in the template.
+    scheduleOnce('afterRender', this, this.insertFilterValueSelector);


I wanted to specifically point out this change since it feels kind of like a hax, but we get double-render errors since this method (the contents of insertFilterValueSelector) was originally moved from didInsertElement to init. We need to wait until the element is rendered to run this, and this feels like the best way to achieve a didInsertElement without actually using the hook 😬

This is tagless and the main blocks of the template are <PolarisStack>s, so we can't have {{did-insert}} modifiers on them because they're components (it works, but tests will error since a modifier on a component is not really acceptable and a bad practice).

Interesting, thanks for the explanation @tomnez! Maybe worth adding as a comment, unless @vladucu has a suggestion for getting around this?

@tomnez I don't think it's that bad what you have here
one thought we could check....is to just have a simple <span {{did-insert ...}}></span> element added (the span should not cause any spacing issues I think)

Yeah that works too 👍I wasn't sure if we wanted to go adding extra nodes that weren't in the React version but I'd rather use the span if we're all cool with it. Thanks ya'll.

andrewpye

Big job here @tomnez, great work muscling through this! Couple of comments but mostly lookin' gooooood 🙌

andrewpye · 2020-02-14T07:06:29Z

addon/components/polaris-resource-list.js

-      if (!this.get('selectable')) {
-        return;
-      }
+  @(computed('selectable', 'sortOptions.length', 'alternateTool').readOnly())


Do we need the .readOnly() here (and in some other places) if this CP is defined without a setter? 🤔

Hm this was all codemod magic, we've been keeping them in other branches. @vladucu any strong feelings on dropping readOnly or not for CP's without setters?

I have no strong opinion here.....I have not commented on these in any PRs, just for the sake of getting these tasks done asap

we'll do some more tweaks at some point in the near future if we want to move to glimmer components with the new tracking feature and we can do at that point too

andrewpye · 2020-02-14T07:19:02Z

addon/components/polaris-resource-list/filter-control/date-selector.js


-  dateComparatorOptions: computed(function() {
+  get dateComparatorOptions() {


Can this just be defined as a regular property now?

yeap, I think we can....nice catch 👍

🤦‍♂ 👍

andrewpye · 2020-02-14T07:20:47Z

addon/components/polaris-resource-list/filter-control/date-selector.js


-  now: computed(function() {
+  get now() {


Does this value get memoized the same way the CP used to? 🤔

not without a @computed decorator

Hmm OK... so we probably want dat @computed decorator here then to maintain behaviour

andrewpye · 2020-02-14T07:26:41Z

addon/components/polaris-resource-list/filter-control/filter-value-selector.js

+    // Using `afterRender` here instead of `didInsertElement` hook.
+    // This component is tagless and has no container div so we can't
+    // use `{{did-insert}}` modifiers in the template.
+    scheduleOnce('afterRender', this, this.insertFilterValueSelector);


Interesting, thanks for the explanation @tomnez! Maybe worth adding as a comment, unless @vladucu has a suggestion for getting around this?

andrewpye · 2020-02-14T07:38:30Z

addon/templates/components/polaris-resource-list/bulk-actions/bulk-action-button.hbs

-  {{/component}}
+  {{#let (element (if @disclosure "span" "")) as |OuterWrapper|}}
+    <OuterWrapper
+      class="Polaris-ResourceList-BulkActions__ActionContent"


Just wanna check that this doesn't have any weird behaviours if we pass a class to OuterWrapper when it's tagless - guessing it just silently does nothing 🤷‍♂

Yeah I tested manually and it just doesn't create the span or a class.

andrewpye · 2020-02-14T07:40:07Z

addon/templates/components/polaris-resource-list/bulk-actions/bulk-action-button.hbs

-    {{#if disclosure}}
-      <span class="Polaris-ResourceList-BulkActions__ActionIcon">
-        {{polaris-icon source="caret-down"}}
-      </span>
-    {{/if}}


Have we lost this block somehow? 😕

What the dang heck. Not sure what happened, will add back 😳

addon/templates/components/polaris-resource-list.hbs

vladucu · 2020-02-17T11:03:33Z

addon/templates/components/polaris-resource-list/bulk-actions/bulk-action-button.hbs

-      </span>
-    {{/if}}
-  {{/component}}
+  {{#let (element (if @disclosure "span" "")) as |OuterWrapper|}}


vladucu · 2020-02-17T11:07:54Z

addon/templates/components/polaris-resource-list/checkable-button.hbs

-  }}
+{{!-- template-lint-disable no-invalid-interactive --}}
+<div
+  {{on "click" this.onToggleAll}}


let's follow what you did in other places

Suggested change

{{on "click" this.onToggleAll}}

{{on "click" (fn (or @onToggleAll this.onToggleAll))}}

vladucu · 2020-02-17T11:09:47Z

addon/components/polaris-resource-list/filter-control/date-selector.js


-  dateComparatorOptions: computed(function() {
+  get dateComparatorOptions() {


yeap, I think we can....nice catch 👍

vladucu · 2020-02-17T11:10:18Z

addon/components/polaris-resource-list/filter-control/date-selector.js


-  now: computed(function() {
+  get now() {


not without a @computed decorator

vladucu · 2020-02-17T11:31:16Z

addon/components/polaris-resource-list/filter-control/filter-value-selector.js

+    // Using `afterRender` here instead of `didInsertElement` hook.
+    // This component is tagless and has no container div so we can't
+    // use `{{did-insert}}` modifiers in the template.
+    scheduleOnce('afterRender', this, this.insertFilterValueSelector);


@tomnez I don't think it's that bad what you have here
one thought we could check....is to just have a simple <span {{did-insert ...}}></span> element added (the span should not cause any spacing issues I think)

vladucu · 2020-02-17T11:34:39Z

addon/templates/components/polaris-resource-list/item.hbs

-          {{/wrapper-element}}
-        {{/wrapper-element}}
-      {{/if}}
+  {{#with (if this.loading "-1" "0") as |tabIndex|}}


let's use let

vladucu · 2020-02-17T11:36:51Z

addon/templates/components/polaris-resource-list/item.hbs

+        @onBlur={{fn this.handleFocusedBlur}}
+      />
+    {{else}}
+      {{#let (element "button") as |ItemButton|}}


do we need this let here? or can we just use the button directly below

I think you missed this comment ^

vladucu · 2020-02-17T11:38:12Z

addon/templates/components/polaris-resource-list/item.hbs

+          aria-controls={{@ariaControls}}
+          aria-expanded={{@ariaExpanded}}
+          tab-index={{tabIndex}}
+          onclick={{fn this.handleClick}}


if this is just a plain button element, can we use {{on "click" ...}} modifier here

vladucu · 2020-02-17T11:50:12Z

addon/templates/components/polaris-resource-list/item.hbs

-      </PolarisButtonGroup>
-    {{/wrapper-element}}
+    {{#if (and @shortcutActions (not this.loading))}}
+      {{#let (element "div") as |ItemActions|}}


again, for these other let's....can't see a reason why we're doing this....unless I'm wrong and missing something let's just use the elements directly
cc @andrewpye in case you remember why we do this

Hm yeah I was just replacing all wrapper-elements with the element helper.. but you're right I can't think of why we would have been using wrapper-element here in the first place since the tagName never changes 🤔Will see if @andrewpye can chime in here but seems like we can drop them now.

vladucu · 2020-02-17T18:29:45Z

addon/components/polaris-resource-list/bulk-actions.js

-      },
-    },
+  @action
+  handleMeasurement(width) {


nice....that previous destructuring was really useless 👏

vladucu · 2020-02-17T18:38:49Z

addon/templates/components/polaris-resource-list/bulk-actions.hbs

      </div>
    {{/if}}

    <button
      class="Polaris-ResourceList-BulkActions__Button Polaris-ResourceList-BulkActions__Button--cancel"
-      onclick={{action "setSelectMode" false}}
+      onclick={{fn this.setSelectMode false}}


can we use on modifier here ?

vladucu · 2020-02-17T18:41:19Z

addon/templates/components/polaris-resource-list/filter-control/filter-value-selector.hbs

-    onFilterKeyChange=(action onFilterKeyChange)
-  }}
-{{/if}}
+<span {{did-insert (fn this.insertFilterValueSelector)}}>


nit: no need for passing fn everywhere, only if you need to curry arguments

vladucu · 2020-02-17T18:42:00Z

addon/templates/components/polaris-resource-list/item.hbs

+        @onBlur={{fn this.handleFocusedBlur}}
+      />
+    {{else}}
+      {{#let (element "button") as |ItemButton|}}


I think you missed this comment ^

vladucu · 2020-02-17T18:42:32Z

addon/templates/components/polaris-resource-list/item.hbs

+    {{#if (or @media this.selectable)}}
+      <div class="Polaris-ResourceList-Item__Owned">
+        {{#if this.selectable}}
+          {{#let (element "div") as |ItemHandle|}}


same....all these let (element... where there's no condition....we can just use the element tag directly no?

vladucu · 2020-02-17T18:42:42Z

addon/templates/components/polaris-resource-list/item.hbs

+            <ItemHandle
+              data-test-id="larger-selection-area"
+              class="Polaris-ResourceList-Item__Handle"
+              onclick={{fn this.handleLargerSelectionArea}}


on render modifier

vladucu · 2020-02-17T18:43:05Z

addon/templates/components/polaris-resource-list/item.hbs

+              {{#let (element "div") as |CheckboxWrapper|}}
+                <CheckboxWrapper
+                  class="Polaris-ResourceList-Item__CheckboxWrapper"
+                  onclick={{fn this.stopPropagation}}


same, we can use div directly here and use on modifier

vladucu

🙌

tomnez · 2020-02-18T15:09:07Z

Thanks guys! 💪

tomnez added 6 commits February 10, 2020 19:00

Run codemods on resource list components

e10774a

Manual edits to component files

bc81ec1

Manual edits to template files

68ab2b3

Use element helper instead wrapper component

d514237

Disable no-invalid-interactive where needed

c52524c

Fix tests

0053ae0

tomnez self-assigned this Feb 12, 2020

tomnez requested review from andrewpye and vladucu February 12, 2020 21:59

tomnez force-pushed the refactor/es6-polaris-resource-list-DEV-78 branch from ce71290 to 346b477 Compare February 13, 2020 16:04

Remove modifiers on components

3f755d9

tomnez force-pushed the refactor/es6-polaris-resource-list-DEV-78 branch from 346b477 to 3f755d9 Compare February 13, 2020 16:23

tomnez commented Feb 13, 2020

View reviewed changes

andrewpye reviewed Feb 14, 2020

View reviewed changes

vladucu reviewed Feb 17, 2020

View reviewed changes

PR edits

2a8ca69

tomnez requested review from andrewpye and vladucu February 17, 2020 18:10

vladucu reviewed Feb 17, 2020

View reviewed changes

Remove element wrappers, use modifiers

e08b111

tomnez requested a review from vladucu February 17, 2020 22:10

vladucu approved these changes Feb 18, 2020

View reviewed changes

tomnez merged commit d3fe8f6 into refactor/es6-classes Feb 18, 2020

tomnez deleted the refactor/es6-polaris-resource-list-DEV-78 branch February 18, 2020 15:09

vladucu added the internal label Jun 26, 2020


		dateComparatorOptions: computed(function() {
		get dateComparatorOptions() {

	{{on "click" this.onToggleAll}}
	{{on "click" (fn (or @onToggleAll this.onToggleAll))}}

Modernize polaris-resource-list components #502

Modernize polaris-resource-list components #502

Conversation

tomnez commented Feb 12, 2020

tomnez commented Feb 12, 2020

andrewpye commented Feb 13, 2020

tomnez commented Feb 13, 2020

tomnez Feb 13, 2020 • 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

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

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

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 18, 2020

tomnez Feb 13, 2020 •

edited