Add data table component #141

andrewpye · 2018-06-22T14:58:47Z

Depends on #142 in order to be able to render arbitrary components in table cells.

Adds full implementation of the Polaris data table component:

Reproducing the demos on the Shopify component documentation page:

Data table with footer:

Data table with sortable columns (when the sorting updates without clicking it's because I pressed Enter while the column heading was focussed):

andrewpye · 2018-06-22T15:01:12Z

Implement Polaris data-table component

…sponsiveness and testing

…errors

…Ember

tomnez

This is awesome! Few questions and comments, but overall this is great and I'm excited to finally get this in 🕺

tomnez · 2018-06-25T15:08:46Z

addon/components/polaris-data-table/cell.js

+
+  /**
+   * @property fixed
+   * @type {boolean}


You have a mix of uppercase and lowercase @types in these files

Yeah... and across the whole addon 😬 If we ever get the stage of using the JSDoc to generate documentation we should definitely standardise this, but at this point I'm not that fussed. Curious as to @vladucu's thoughts on this...

He'd probably say something like this 😉

Yeah maybe 😛 We currently have 22 {Boolean}s and 54 {boolean}s, so clearly we'll need to tidy those up at some point 😬 I think it's a big enough job to be done separately from this PR 🤷‍♂️

Yeah I just meant change the files in this PR, not across the addon. No reason to continue a bad habit just because it's already been done elsewhere.

I don't really care much about these at this point since we don't use them....but going forward we could try to add them appropriately - uppercased?

tomnez · 2018-06-25T15:29:58Z

addon/helpers/get-array-item.js

+// to the `get` helper. If we stop supporting
+// Ember 2.12, we can remove this helper and
+// replace its usages with the existing `get`
+// helper.


tomnez · 2018-06-25T15:36:53Z

addon/components/polaris-data-table/total.js

+  layout,
+
+  /**
+   * TODO: make this support passing a component to render instead of just text/numbers.


Is this note still relevant since we are using {{render-content ...}} in this branch now?

Good catch, I rushed through adding render-content yesterday afternoon and missed this 👍

addon/templates/components/polaris-data-table/cell.hbs

+          aria-sort={{sortDirection}}
+          aria-label={{sortAccessibilityLabel}}
+          tabindex="0"
+          {{action onSort}}


addon/templates/components/polaris-data-table/cell.hbs

+          {{action onSort}}
+          onkeydown={{action "onKeyDown"}}
+        >
+          {{#if (eq contentType "text")}}


tomnez · 2018-06-25T16:10:57Z

addon/templates/components/polaris-data-table/navigation.hbs

+}}
+
+{{#each columnVisibilityData as |column|}}
+  <div class="Polaris-DataTable__Pip {{if column.isVisible "Polaris-DataTable__Pip--visible"}}"></div>


Maybe single-quote the inner string for slightly easier readability? No biggie if we prefer double quotes even for nested strings in templates, but just a thought.

Fair point, never single-quoted in template files before. I'm OK with this as is, but if both you and @vladucu would prefer it changed I'm happy to change it, no biggie as you say 😉

I've usually used single-quotes in cases like this just to avoid confusion and be easier to read

tomnez · 2018-06-25T16:16:30Z

tests/integration/components/polaris-data-table-test.js

+    assert.ok(fifthHeadingCell.classList.contains('Polaris-DataTable__Cell--sorted'));
+  });
+
+  test('it accepts both text and component definitions as cell contents', async function(assert) {


💯 💯 💯

tomnez · 2018-06-25T16:24:28Z

addon/components/polaris-data-table/cell.js

+   * @type {boolean}
+   * @public
+   */
+  presentational: false,


There are some inconsistencies in these files on whether or not there's a @default added - I think we want @default notation for any non-null attribute, yes?

tomnez · 2018-06-25T16:29:30Z

addon/components/polaris-data-table.js

+  },
+
+  handleResize() {
+    // This is needed to replicate the React implementation's `@debounce` decorator.


tomnez · 2018-06-25T16:51:06Z

addon/components/polaris-data-table.js

+          previousColumn.leftEdge < 10 ? 0 : previousColumn.leftEdge;
+      }
+
+      // TODO: use run loop instead of `requestAnimationFrame` here?


Seems like requestAnimationFrame is just an overkill way of quickly calculating precise position values of columns? I don't know if swapping for run loop would accomplish whatever the intention of requestAnimationFrame here is, maybe just leave as-is unless you're certain that we need to throw this into one of the run loop cycles.

My understanding is that requestAnimationFrame helps prevent jankiness on scrolling, resizing etc. by limiting the amount of work done to only that which can be fitted between two consecutive frame renders. With this in mind, using it here makes some kind of sense, although I'm slightly confused as to why it's not being used anywhere else (I'd expect the resize handler to be a candidate for its use, for example).

That being said, I'm not sure if using it directly here creates the possibility of interfering with the Ember run loop. The interesting-looking ember-run-raf addon seems to be advocating "using run.scheduleOnce + requestAnimationFrame" in combination, so maybe we should be doing that combo here, not sure...

Oh wow I didn't realize there was an addon that tied the two together. If it's owned/authored by runspired then I trust it, but again for this purpose I don't think it's really an urgent need. It's up to you if you want to experiment with it but I think I'm fine to leave it as-is.

Yeah agreed, that guy knows what he's doing (though I bet he wishes he was @vladucu 😉)! I wasn't intending to pull in the addon here though, just found it the other day and thought it was interesting.

add a note here maybe to check ember-run-raf when testing this in the real world?

From what I can see, ember-run-raf changes the entire run loop to use requestAnimationFrame rather than adding the option to e.g. schedule on an animation frame queue, so probs not something we want to pull in to the host app 😬

… TODO.

vladucu · 2018-06-26T13:03:35Z

sorry for the delay on this, getting to it in a bit...

vladucu

this looks pretty nice!

a couple of small comments, mostly I'd like us to clean up event handling before this goes out

vladucu · 2018-06-27T10:15:16Z

addon/components/polaris-data-table/cell.js

+
+  /**
+   * @property fixed
+   * @type {boolean}


I don't really care much about these at this point since we don't use them....but going forward we could try to add them appropriately - uppercased?

vladucu · 2018-06-27T10:34:55Z

addon/templates/components/polaris-data-table/cell.hbs

+          style={{style}}
+          role="button"
+          aria-sort={{sortDirection}}
+          aria-label={{sortAccessibilityLabel}}


need to define sortAccessibilityLabel, check here for their locale

vladucu · 2018-06-27T10:42:58Z

addon/templates/components/polaris-data-table/cell.hbs

+          onkeydown={{action "onKeyDown"}}
+        >
+          {{#if (eq contentType "text")}}
+            <span class="Polaris-DataTable__Heading {{if (eq contentType "text") "Polaris-DataTable__Heading--left"}}">


this is already in a if (eq contentType "text"), can we just add "Polaris-DataTable__Heading--left" class without any condition?

vladucu · 2018-06-27T10:48:13Z

addon/templates/components/polaris-data-table/cell.hbs

+              </span>
+            </span>
+          {{else}}
+            <span class="Polaris-DataTable__Heading {{if (eq contentType "text") "Polaris-DataTable__Heading--left"}}">


and here being in the if (eq contentType "text") else branch, don't add "Polaris-DataTable__Heading--left" at all

vladucu · 2018-06-27T11:06:03Z

addon/templates/components/polaris-data-table/navigation.hbs

+}}
+
+{{#each columnVisibilityData as |column|}}
+  <div class="Polaris-DataTable__Pip {{if column.isVisible "Polaris-DataTable__Pip--visible"}}"></div>


I've usually used single-quotes in cases like this just to avoid confusion and be easier to read

vladucu · 2018-06-27T11:38:24Z

addon/components/polaris-data-table.js

+        index,
+      );
+    }
+    return {leftEdge, rightEdge, isVisible};


vladucu · 2018-06-27T11:49:57Z

addon/components/polaris-data-table.js

+
+    if (!truncate) {
+      return (heights = rows.map((row) => {
+        let fixedCell = row.children[0];


react uses childNodes, any reason we don't follow?

Yep, childNodes includes a bunch of extra text nodes in Ember land which aren't present in the React world (spent a lil while tracking down why this wasn't behaving properly and that turned out to be the cause) 🤷‍♂️

vladucu · 2018-06-27T11:55:42Z

addon/components/polaris-data-table.js

+          previousColumn.leftEdge < 10 ? 0 : previousColumn.leftEdge;
+      }
+
+      // TODO: use run loop instead of `requestAnimationFrame` here?


add a note here maybe to check ember-run-raf when testing this in the real world?

vladucu · 2018-06-27T12:00:55Z

addon/components/polaris-data-table.js

+   * @type {HTMLElement}
+   * @private
+   */
+  table: volatileElementLookup('.Polaris-DataTable__Table').readOnly(),


I don't think these element lookups need to be volatile

I guess you're right actually 👍

vladucu · 2018-06-27T12:02:10Z

tests/integration/components/polaris-data-table-test.js

+import { setupRenderingTest } from 'ember-qunit';
+import { render } from '@ember/test-helpers';
+import hbs from 'htmlbars-inline-precompile';
+import { findAll, find } from 'ember-native-dom-helpers';


we should use the new find, findAll from @ember/test-helpers....we can probably start deprecating native-dom helpers

not saying this should not go out because of that^

we should also use qunit-dom for assertion, from what I've seen much nicer error messages

…m-helpers

andrewpye · 2018-06-27T13:21:41Z

@vladucu changes pushed, ready for 👀 pt. deux 😅

andrewpye · 2018-06-27T13:31:18Z

(tests are currently failing on Travis due to package issues 😞They all pass locally!)

vladucu

👏

vladucu · 2018-06-27T13:46:53Z

addon/components/polaris-data-table/cell.js

@@ -188,6 +209,17 @@ export default Component.extend({
    return `caret-${ sortDirection === 'ascending' ? 'up' : 'down' }`;
  }).readOnly(),

+  updateAccessibilityLabel() {
+    let cellElement = document.querySelector(`#${ this.get('cellElementId') }`);
+    this.set('sortAccessibilityLabel', `Sort by ${cellElement.textContent.trim().toLowerCase()}`);


nit: spaces

…he header

andrewpye self-assigned this Jun 22, 2018

andrewpye requested review from vladucu and tomnez June 22, 2018 14:58

andrewpye added 12 commits June 25, 2018 17:44

Initial minimal polaris-data-table implementation - needs sorting, re…

96f4a48

…sponsiveness and testing

Hook up table responsiveness, column scrolling etc.

e191553

Add sorting support to polaris-data-table

63bacdf

Add basic polaris-data-table tests and fix a couple of minor oopsies

3966981

Fix test failures and prevent 'calling set on destroyed object' test …

f8e44d5

…errors

Fix Ember 2.12 errors caused by passing integer to 'get' helper

1043626

Support passing components to data table cells

36dfc1f

Fix 'object is not extensible' test errors on some versions of Ember

f79fbb4

Try fixing data table compatibility issues with pre-2.18 versions of …

1efcf3e

…Ember

Add documentation and changelog entry for polaris-data-table

a6d270d

Use render-content to clean up using components in data table

97a4b06

Remove done TODO

3c821b0

andrewpye force-pushed the add-data-table-component branch from 73062c9 to 3c821b0 Compare June 25, 2018 14:48

andrewpye changed the base branch from master to enhancement/support-react-style-content-rendering June 25, 2018 14:49

tomnez reviewed Jun 25, 2018

View reviewed changes

andrewpye changed the base branch from enhancement/support-react-style-content-rendering to master June 26, 2018 06:35

Add some @default values to JSDocs where appropriate. Remove outdated…

787c22c

… TODO.

vladucu requested changes Jun 27, 2018

View reviewed changes

andrewpye added 5 commits June 27, 2018 15:24

Fix table cell accessibility label

8852260

Use ember-lifeline for binding event handlers

0fc8b05

Remove volatile modifier from element lookup CPs. Minor style tweaks.

35be0af

Use find, findAll from @ember/test-helpers instead of ember-native-do…

fc28909

…m-helpers

Use ember-lifeline to simplify debouncing event handlers

c265680

andrewpye requested a review from vladucu June 27, 2018 13:21

andrewpye requested a review from tomnez June 27, 2018 13:21

vladucu approved these changes Jun 27, 2018

View reviewed changes

andrewpye added 3 commits June 27, 2018 17:10

Tweak spaces after review

19113d1

Fix case of sortable table heading aria-label

0811f4c

Fix sortable table heading chevron color being black after clicking t…

ffd3d39

…he header

andrewpye merged commit 0aef05d into master Jun 28, 2018

andrewpye deleted the add-data-table-component branch June 28, 2018 12:06

Add data table component #141

Add data table component #141

Conversation

andrewpye commented Jun 22, 2018 • edited

andrewpye commented Jun 22, 2018

tomnez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomnez Jun 26, 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

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

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 commented Jun 26, 2018

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

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 commented Jun 27, 2018

andrewpye commented Jun 27, 2018

vladucu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewpye commented Jun 22, 2018 •

edited

tomnez Jun 26, 2018 •

edited