Allow row selection in fixed tables #23

AdamWard1995 · 2017-06-29T16:32:30Z

This project uses semver, please check the scope of this pr:

#none# - documentation fixes and/or test additions
#patch# - backwards-compatible bug fix
#minor# - adding functionality in a backwards-compatible manner
#major# - incompatible API change

Add functionality for selecting table rows to frost-fixed-table. Needed for an internal story.

CHANGELOG

Added functionality for selecting table rows in frost-fixed-table

AdamWard1995 · 2017-06-29T16:37:12Z

Realized that I left changes in the demo I didn't want to, will remove those

dafortin · 2017-06-29T18:41:26Z

addon/components/frost-fixed-table.js

@@ -20,12 +21,29 @@ export default Component.extend({
  // == PropTypes =============================================================

  propTypes: {
-    // required:
+    // options:


Any reason to rename this? Consistency with other existing components?

Yes, consistency with frost-table

dafortin · 2017-06-29T18:46:03Z

addon/components/frost-fixed-table.js

@@ -76,6 +99,17 @@ export default Component.extend({
  @readOnly
  @computed('css')
  /**
+   * The selector for the left header DOM element
+   * @param {String} css - the base css class name for the component
+   * @returns {String} a sutiable jQuery selector for the left section of the table header


typo suitable?

dafortin · 2017-06-29T18:46:09Z

addon/components/frost-fixed-table.js

+
+  @readOnly
+  @computed('css')
+  /**
   * The selector for the middle header DOM element (specifically the scroll wrapper)
   * @param {String} css - the base css class name for the component
   * @returns {String} a sutiable jQuery selector for the middle section of the table header


typo suitable?

dafortin · 2017-06-29T18:46:35Z

addon/components/frost-fixed-table.js

+  /**
+   * The selector for the right header DOM element
+   * @param {String} css - the base css class name for the component
+   * @returns {String} a sutiable jQuery selector for the right section of the table header


typo suitable?

dafortin · 2017-06-29T18:48:55Z

addon/components/frost-fixed-table.js

+   * @param {String} css - the base css class name for the component
+   * @returns {String} a sutiable jQuery selector for the left section of the table header
+   */
+  headerLeftSelector (css) {


Are we exposing this CP to the consumer? I'm simply wondering because there is an inconsistency in naming CP _ vs no _ for internal CPs. No big deal I'm simply wondering.

^ is a valid comment for all other CPs

dafortin · 2017-06-29T19:39:40Z

addon/components/frost-fixed-table.js

  /**
   * Set up synced scrolling as well as calculating padding for middle sections
   */
  didRender () {
    this._super(...arguments)
    this.setupBodyHeights()
    this.setupHoverProxy()
+    this.setupLeftAndRightWidths() // Needs to happen before setting up middle section


Why do we need this now?

Is this related to the selection?

Adding in the checkboxes column misaligns the columns BAD

dafortin · 2017-06-29T19:43:44Z

addon/components/frost-fixed-table.js

+      const clonedSelectedItems = A(this.get('selectedItems').slice())
+      const _rangeState = this.get('_rangeState')
+
+      // Selects are proccessed in order of precedence: specific, range, basic


I think this should be isolated in a util since it can be common.

dafortin · 2017-06-29T19:49:13Z

addon/components/frost-table-row.js

@@ -65,6 +65,11 @@ export default Component.extend({

  // == DOM Events ============================================================

+  click (event) {
+    event.stopPropagation()
+    this.$().find('.frost-table-row-selection').trigger('click')


Can we simply trigger consumer action? (not sure here I'm simply wondering)

So frost-table-row when you enable selection adds an frost-table-row-selection to the row. It is within the click event handler of frost-table-row-selection that the onSelect callback is call, thought it would be simplist to just trigger this event

Are we doing an action on click for the table on when we click on a button or something like that (similar to list)? If we are using a button to do an action based on selection then we should use a similar implementation to the list and I'm not seeing anything like this in the list.

dafortin · 2017-06-29T19:49:56Z

addon/templates/components/frost-fixed-table.hbs

@@ -31,8 +31,10 @@
      hook=(concat hookPrefix '-header-left')
      hookQualifiers=hookQualifiers
      tagName='div'
+      isSelectable=_isSelectable


We can select the header? For sorting I'm expecting?

This was done in frost-table, for sorting was my guess as well but will look a little deeper to confirm

Not for sorting, this is how the header knows to add in a column in header that corresponds to the checkbox column

dafortin · 2017-06-29T19:53:19Z

app/styles/ember-frost-table/_frost-fixed-table.scss

+
+    // sass-lint:disable nesting-depth
+    .frost-table-row {
+      &.is-selected {


Are we using is-selected in the list for selection? It will be nice to use the same class name.

notmessenger · 2017-06-30T12:20:35Z

.travis.yml

@@ -17,7 +17,6 @@ addons:

 cache:
  directories:
-  - node_modules


@AdamWard1995 why? This does not follow the practice of any of our other repos /cc @juwara0

@notmessenger - is correct. Please do not remove the caching.

He's trying to figure out why something isn't working, and needed to clear the cache. It should be temporary.

Going to add it back, just clearing it as locally everything is installing fine :)

That setting tells travis that it can cache the node_modules directories during builds. https://docs.travis-ci.com/user/caching/#Caching-directories-(Bundler%2C-dependencies). If you need to clear the cache during CI there is an option to do so on the build page under the drop-down in the upper right corner.

notmessenger · 2017-06-30T12:32:53Z

ember-cli-build.js

@@ -24,8 +24,6 @@ module.exports = function (defaults) {

  if (app.env === 'test') {


The rest of this code

if (app.env === 'test') { ;[ ].forEach((path) => { app.import(path, {type: 'test'}) }) }

needs to be removed as well

coveralls · 2017-06-30T12:33:49Z

Changes Unknown when pulling 28901f7 on AdamWard1995:fixedTableSelection into ** on ciena-frost:master**.

coveralls · 2017-06-30T15:33:54Z

Changes Unknown when pulling 49b0844 on AdamWard1995:fixedTableSelection into ** on ciena-frost:master**.

coveralls · 2017-07-04T18:27:13Z

Changes Unknown when pulling a75f7ea on AdamWard1995:fixedTableSelection into ** on ciena-frost:master**.

coveralls · 2017-07-04T18:50:54Z

Changes Unknown when pulling 461689f on AdamWard1995:fixedTableSelection into ** on ciena-frost:master**.

dafortin · 2017-07-04T19:19:56Z

Looks good :) 👍

Allow row selection in fixed tables

8faef03

dafortin reviewed Jun 29, 2017

View reviewed changes

AdamWard1995 added 2 commits June 30, 2017 07:57

Attempt to fix dependency issues

e606340

Clear node_modules cache

fe79905

notmessenger reviewed Jun 30, 2017

View reviewed changes

Remove import of old bower_components

28901f7

notmessenger reviewed Jun 30, 2017

View reviewed changes

AdamWard1995 added 2 commits June 30, 2017 11:14

Fix review comments

8bd90ff

Fix linting issue

49b0844

Fix some UI problems

a75f7ea

Extact click handler into utility method

461689f

dafortin approved these changes Jul 4, 2017

View reviewed changes

dafortin merged commit 06b8646 into ciena-frost:master Jul 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow row selection in fixed tables #23

Allow row selection in fixed tables #23

AdamWard1995 commented Jun 29, 2017

AdamWard1995 commented Jun 29, 2017

dafortin Jun 29, 2017

AdamWard1995 Jun 30, 2017

dafortin Jun 29, 2017

dafortin Jun 29, 2017

dafortin Jun 29, 2017

dafortin Jun 29, 2017

dafortin Jun 29, 2017

dafortin Jun 29, 2017

dafortin Jun 29, 2017

AdamWard1995 Jun 30, 2017

dafortin Jun 29, 2017

dafortin Jun 29, 2017

AdamWard1995 Jun 30, 2017

dafortin Jul 4, 2017

dafortin Jun 29, 2017

AdamWard1995 Jun 30, 2017

AdamWard1995 Jun 30, 2017

dafortin Jun 29, 2017

AdamWard1995 Jun 30, 2017

notmessenger Jun 30, 2017

juwara0 Jun 30, 2017

job13er Jun 30, 2017

AdamWard1995 Jun 30, 2017

juwara0 Jun 30, 2017

notmessenger Jun 30, 2017

AdamWard1995 Jun 30, 2017

coveralls commented Jun 30, 2017

coveralls commented Jun 30, 2017

coveralls commented Jul 4, 2017

coveralls commented Jul 4, 2017

dafortin commented Jul 4, 2017

		@@ -24,8 +24,6 @@ module.exports = function (defaults) {

		if (app.env === 'test') {

Allow row selection in fixed tables #23

Allow row selection in fixed tables #23

Conversation

AdamWard1995 commented Jun 29, 2017

This project uses semver, please check the scope of this pr:

CHANGELOG

AdamWard1995 commented Jun 29, 2017

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

coveralls commented Jun 30, 2017

coveralls commented Jun 30, 2017

coveralls commented Jul 4, 2017

coveralls commented Jul 4, 2017

dafortin commented Jul 4, 2017