Selection #13
Selection #13
Conversation
|
Realized with a clearer brain this morning that the interface can be simplified a bit, I'll make the updates shortly |
… not shift the cells
|
Raised an issue to move the selection util into a common package |
addon/components/frost-table-body.js
Outdated
| } | ||
| return '@index' | ||
| }, | ||
|
|
There was a problem hiding this comment.
I'm assuming this can be done with getDefaultProps().
There was a problem hiding this comment.
Excellent point sir
There was a problem hiding this comment.
Hmm actually, I'll have to make sure I can safely use itemKey instead of the targeted eachItemKey, it may bleed across purposes
| // == DOM Events ============================================================ | ||
|
|
||
| click (event) { | ||
| this.onSelectionChange([]) |
There was a problem hiding this comment.
Shouldn't really matter, but doesn't hurt, so sure
There was a problem hiding this comment.
Potential benefits if consumer disables prototype extension. :)
| PropTypes.EmberObject, | ||
| PropTypes.object | ||
| ]) | ||
| }) | ||
| }, | ||
|
|
||
| getDefaultProps () { | ||
| return { | ||
| // options | ||
| columns: [], | ||
| items: [], |
There was a problem hiding this comment.
They're internal and functional so probably not necessary, but again doesn't hurt so sure
| this._super(...arguments) | ||
| const itemKey = this.get('itemKey') | ||
| if (itemKey) { | ||
| this.set('_itemComparator', function (lhs, rhs) { |
There was a problem hiding this comment.
_itemComparator will never be set if itemKey is missing. I saw itemKey is assigned a default value inside frost-table-row. It's probably better to move the logic here and pass itemKey down.
There was a problem hiding this comment.
Nope - different purposes - the CP in the body is to handle optimization of the each if no itemKey is provided. This case is present only for selection/expansion, so it's actually two different cases
There was a problem hiding this comment.
Sure. I see they serve a different purpose. but itemKey should be a required prop (It's an option in prop type now). Otherwise, if consumer invokes the component without itemKey, and then try to select an item. It apparent will fail since _select action in frost-table and _isItemSelected cp in frost-table-row will try to run _itemComparator but no such method exists. And if itemKey is required, there's no need for providing a default in frost-table-row.
I could be missing something obvious, but I'm not following why the existence of _itemComparator is bound to itemKey. And actually, I feel itemKey should be optional and default to id if the consumer doesn't provide in frost-table. And the {{#each }} in frost-table-body can pick up the itemKey we passed in as its loop key.
There was a problem hiding this comment.
- Selection isn't required for a table (or list) so an itemKey isn't required.
idis not a guaranteed property_itemComparatoris bound toitemKeybecause we want to compare for equality using that key most of the time instead of attempting a strict comparison on the object itself
|
Changes Unknown when pulling 6cba76e on sglanzer:master into ** on ciena-frost:master**. |
|
Changes Unknown when pulling 9f92943 on sglanzer:master into ** on ciena-frost:master**. |
|
Approved |
This project uses semver, please check the scope of this pr:
Adds a selection column to the table if selection bindings are provided. Follows the same selection conventions as ember-frost-list, but limits the selectable area to the selection column cell (represented with a checkbox)
CHANGELOG