Comparator functionality #102

frosetti · 2017-02-15T18:37:42Z

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

#patch# - backwards-compatible bug fix
#minor# - adding functionality in a backwards-compatible manner
#major# - incompatible API change

CHANGELOG

Added a default comparator which can be overridden with a custom comparator.
Added tests for three scenarios of clicking
Modified frost-list template to have a hook at the top of the container for easier testing of items below it.

coveralls · 2017-02-15T18:50:47Z

Coverage increased (+71.7%) to 74.038% when pulling 80534ac on frosetti:fr/itemComparator into 64c4496 on ciena-frost:master.

TheOtherDude · 2017-02-15T18:50:34Z

addon/components/frost-list.js

+          expandedItem => itemComparator(expandedItem, item)) >= 0)
+      set(item, 'isSelected',
+        isEmpty(selectedItems) ? false : selectedItems.findIndex(
+          selectedItem => itemComparator(selectedItem, item)) >= 0)


ItemComparator should return a boolean, not an index, so you should go with something more like this:

set(item, 'isSelected', isEmpty(selectedItems) ? false : selectedItems.some(selectedItem => itemComparator(selectedItem, item)))

I switched it over to use the .some function now.

TheOtherDude · 2017-02-15T18:53:33Z

addon/utils/selection.js

      const isSelected = !isCurrentlySelected
      if (isSelected) {
        selectedItems.pushObject(item)
      } else {
-        selectedItems.removeObject(item)
+        selectedItems.removeAt(index)
+        // selectedItems.removeObject(item)


Remove obsolete code, if we do want to keep your updated code. You already have the item, so I'm not sure what we gain by switching to .removeAt().

Removed it.

TheOtherDude · 2017-02-15T18:54:16Z

addon/utils/selection.js

@@ -88,7 +93,8 @@ export default {

    // If an endpoint was already selected remove selected items that were
    // in the previous range but aren't in the new range
-    const previousEndpoint = items.indexOf(rangeState['endpoint'])
+    // const previousEndpoint = items.indexOf(rangeState['endpoint'])


Remove obsolete code.

Removed it.

TheOtherDude · 2017-02-15T19:00:00Z

addon/utils/selection.js

-    const isCurrentlySelected = selectedItems.indexOf(item) >= 0
+  specific (selectedItems, item, rangeState, itemComparator) {
+    const index = selectedItems.findIndex(selectedItem => itemComparator(selectedItem, item))
+    const isCurrentlySelected = (index >= 0)


You could just do this instead, so you don't have to .splice() later on:

const isCurrentlySelected = selectedItems.some(selectedItem => itemComparator(selectedItem, item)) ... selectedItems.removeObject(item)

Can't use .removeObject in most places where you use a comparator as the objects are not guaranteed to be the same even if they have the same contents.

TheOtherDude · 2017-02-15T19:02:00Z

tests/integration/components/frost-list-test.js

-        ).to.eql(true)
+    describe('when doing basic click', function () {
+      beforeEach(function () {
+        this.$(hook('my-list-item', {index: 0})).click()


You should use $hook() when you want a jQuery object rather than wrapping it in this.$().

I believe I removed out all the this.$

TheOtherDude · 2017-02-15T19:03:00Z

tests/integration/components/frost-list-test.js

+      })
+
+      it('item 0 is selected', function () {
+        expect(this.$($hook('my-list-item-container', {index: 0})).hasClass('is-selected')).to.eql(true)


Please remove the this.$() here and subsequent tests - it's not doing anything for you.

TheOtherDude · 2017-02-15T19:04:29Z

tests/integration/components/frost-list-test.js

-        ).to.eql(false)
+    describe('when doing basic click and all is selected', function () {
+      beforeEach(function () {
+        this.$($(hook('my-list-item-container', {index: 0})).find(hook('my-list-selection-checkbox'))).click()


Like above, you should prefer $hook() over $(hook()).

Also, we should try to just pass the index to the selection checkbox so you can select it with $hook('my-list-selection-checkbox', {index: 0})

This should do the trick in the frost-list-item-selection template:

{{! Template for the frost-list-item-selection component }} {{frost-checkbox checked=isSelected hook=(concat hookPrefix '-checkbox') hookQualifiers=hookQualifiers size='medium' }}

I was unable to get that to work in that template but I did get it to work in
{{frost-list-item-selection
hook=(concat hookPrefix '-selection')
hookQualifiers=(hash index=index)
model=model
onSelect=(action '_select')
}}

TheOtherDude · 2017-02-15T19:09:46Z

addon/templates/components/frost-list.hbs

-    {{if (is-lead-selection _items model) ' is-lead-selection'}}
-  '>
+    {{if (is-lead-selection _items model) ' is-lead-selection'}}'
+       data-test={{hook (concat hook '-item-container') index=index }}


I'd remove a couple spaces here so that data-test aligns with {{if ...}}.

It's aligned now.

frosetti · 2017-02-15T21:02:28Z

~~Don't merge this yet. I have to adjust the expanded items to use the comparator.~~
Added expanded item comparator and test for it.

coveralls · 2017-02-15T21:03:03Z

Coverage increased (+71.7%) to 74.038% when pulling 67d33dd on frosetti:fr/itemComparator into 64c4496 on ciena-frost:master.

coveralls · 2017-02-15T21:10:03Z

Coverage increased (+71.7%) to 74.038% when pulling 67d33dd on frosetti:fr/itemComparator into 64c4496 on ciena-frost:master.

TheOtherDude · 2017-02-15T21:25:32Z

tests/integration/components/frost-list-test.js

@@ -143,11 +143,11 @@ describe(test.label, function () {
    })

    it('item 0 is selected', function () {
-      expect(this.$($hook('my-list-item-container', {index: 0})).hasClass('is-selected')).to.eql(true)
+      expect($($hook('my-list-item-container', {index: 0})).hasClass('is-selected')).to.eql(true)


You are still wrapping these in an extra $().

And it's minor, but since you're going to change this anyway, you could take advantage of jquery-chai and write:

expect($hook('my-list-item-container', {index: 0})).to.have.class('is-selected')

That way you get a better failure message instead of false does not deeply equal true.

coveralls · 2017-02-15T22:12:52Z

Coverage increased (+78.2%) to 80.531% when pulling 5b31f45 on frosetti:fr/itemComparator into 64c4496 on ciena-frost:master.

sglanzer-deprecated · 2017-02-17T05:07:19Z

addon/components/frost-list.js

      const _rangeState = this.get('_rangeState')

      // Selects are proccessed in order of precedence: specific, range, basic
      if (isSpecificSelect) {
-        selection.specific(clonedSelectedItems, item, _rangeState)
+        selection.specific(clonedSelectedItems, item, _rangeState, this.get('itemComparator'))


Future note, best to store this against a const and then refer for each usage (when more than one usage is present)

sglanzer-deprecated · 2017-02-17T05:09:10Z

addon/templates/components/frost-list.hbs

-    {{if (is-lead-selection _items model) ' is-lead-selection'}}
-  '>
+    {{if (is-lead-selection _items model) ' is-lead-selection'}}'
+    data-test={{hook (concat hook '-item-container') index=index }}


Minor - it's clearer if the concat uses hookPrefix to delineate

sglanzer-deprecated · 2017-02-17T05:11:31Z

Very nice PR - well done 👍

Comparator functionality

80534ac

TheOtherDude requested changes Feb 15, 2017

View reviewed changes

frosetti added 2 commits February 15, 2017 12:29

Removed old cold, updated hooks and tests

69f8c71

Fixed lint

67d33dd

TheOtherDude reviewed Feb 15, 2017

View reviewed changes

Added Expanded Items Comparator

5b31f45

TheOtherDude approved these changes Feb 15, 2017

View reviewed changes

TheOtherDude merged commit dd8fef1 into ciena-frost:master Feb 15, 2017

sglanzer-deprecated reviewed Feb 17, 2017

View reviewed changes

This was referenced Feb 17, 2017

pr-bumper never published a new minor for PR 102 #103

Closed

Fix build in order to publish the previous PR as a minor #104

Merged

Fix the build #108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comparator functionality #102

Comparator functionality #102

frosetti commented Feb 15, 2017

coveralls commented Feb 15, 2017 •

edited

Loading

TheOtherDude Feb 15, 2017

frosetti Feb 15, 2017

TheOtherDude Feb 15, 2017

frosetti Feb 15, 2017

TheOtherDude Feb 15, 2017

frosetti Feb 15, 2017

TheOtherDude Feb 15, 2017

frosetti Feb 15, 2017

TheOtherDude Feb 15, 2017

frosetti Feb 15, 2017

TheOtherDude Feb 15, 2017

TheOtherDude Feb 15, 2017 •

edited

Loading

TheOtherDude Feb 15, 2017 •

edited

Loading

frosetti Feb 15, 2017

TheOtherDude Feb 15, 2017

frosetti Feb 15, 2017

frosetti commented Feb 15, 2017 •

edited

Loading

coveralls commented Feb 15, 2017 •

edited

Loading

coveralls commented Feb 15, 2017

TheOtherDude Feb 15, 2017

coveralls commented Feb 15, 2017 •

edited

Loading

sglanzer-deprecated Feb 17, 2017

sglanzer-deprecated Feb 17, 2017

sglanzer-deprecated commented Feb 17, 2017

Comparator functionality #102

Comparator functionality #102

Conversation

frosetti commented Feb 15, 2017

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

CHANGELOG

coveralls commented Feb 15, 2017 • edited Loading

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

TheOtherDude Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

TheOtherDude Feb 15, 2017 • edited Loading

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

frosetti commented Feb 15, 2017 • edited Loading

coveralls commented Feb 15, 2017 • edited Loading

coveralls commented Feb 15, 2017

Choose a reason for hiding this comment

coveralls commented Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sglanzer-deprecated commented Feb 17, 2017

coveralls commented Feb 15, 2017 •

edited

Loading

TheOtherDude Feb 15, 2017 •

edited

Loading

TheOtherDude Feb 15, 2017 •

edited

Loading

frosetti commented Feb 15, 2017 •

edited

Loading

coveralls commented Feb 15, 2017 •

edited

Loading

coveralls commented Feb 15, 2017 •

edited

Loading