Fr/range select #113

frosetti · 2017-03-01T03:08:37Z

Updated

Page testing

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

CHANGELOG

Fixed range select issues with selectedItems increasing with duplicates
Simplified itemComparator to just itemKey: string

sglanzer-deprecated · 2017-03-01T13:48:52Z

README.md

@@ -59,7 +59,7 @@ Detailed API and example usage can be found in the sample application in tests/d
 | `Sub Attribute`    | `activeSorting`      | `array`          |       | Array that specifies the sort order. eg. [{"direction: "asc/desc", "value": <attr-name>}], This is an attribute on frost-list-expansion component.|
 | `Sub Attribute`    | `properties`         | `array`          |       | Array of sortable attributes. eg. [{"label: "foo", "value": "bar"}], This is an attribute on frost-sort component.|
 | `Sub Attribute`    | `onSort`             | `action closure` |       | callback functions user provided to handle sorting.  This is an attribute on frost-sort component.|
-| `Attribute`        | `itemComparator`     | `action closure` |       | callback functions user provided to handle custom item comparisons.|


Technically this removal would be considered a breaking change - I'm going to let it slide this time because I'm confident this hasn't propagated far and you're likely the only one using it, but you should announce this removal on #frost-announcements

Please be careful in the future about choosing appropriate interfaces up front, even if the design phase takes longer

sglanzer-deprecated · 2017-03-01T13:51:44Z

addon/components/frost-list.js

@@ -134,6 +136,20 @@ export default Component.extend({
    }
  },

+  didReceiveAttrs () {


I would put this in the init, not the receive, since there should be no expectation of your itemKey changing

I will update to reflect this.

sglanzer-deprecated · 2017-03-01T13:52:48Z

addon/components/frost-list.js

      }
    }
  },

  // == Computed Properties ===================================================

  @readOnly
-  @computed('expandedItems.[]', 'items.[]', 'selectedItems.[]', 'itemComparator')
+  @computed('expandedItems.[]', 'items.[]', 'selectedItems.[]', '_itemComparator')
  _items (expandedItems, items, selectedItems, itemComparator) {


Probably makes sense for your variable to match the source - i.e. _itemComparator

I saw it was being done earlier so I decided I better copy the flow.

sglanzer-deprecated · 2017-03-01T13:53:50Z

addon/components/frost-list.js

@@ -167,19 +183,24 @@ export default Component.extend({
      this.onExpansionChange(this.get('items'))
    },

+    _onPaginationChange () {


What's the purpose of this?

Originally I was going to write a action that fired on pagination change that would reset the anchors. I then realized the more I stared at the flow of the code, this cause extra complexity and I forgot to remove it before I moved onto a different route. Good catch though.

sglanzer-deprecated · 2017-03-01T13:58:45Z

addon/utils/selection.js

      // Range select is always a positive selection (no deselect)
      rangeState['anchor'] = item

      // New anchor, clear any previous endpoint
      rangeState['endpoint'] = Ember.Object.create()

-      // Add the anchor to the selected items
-      selectedItems.pushObject(item)
+      // Add the anchor to the selected items if not already in it from a previous page


The "previous page" comment has me wary - what scenario are you attempting to cover?

In pagination, the way this code is set up, your anchors should be reset.
If you are on page 2, do some shift clicking.
Switch to page 1, do a shift click on a already selected item. You will add this item into selectedItems due to there was no check.

sglanzer-deprecated · 2017-03-01T14:01:14Z

addon/utils/selection.js

    // If an anchor isn't set, then set the anchor and exit
    const rangeAnchor = rangeState['anchor']
-    if (isNone(rangeAnchor)) {


I've checked the behaviour of the list selection prior to your last set of comparator changes and the behaviour is correct - what case are you addressing here that requires a change to the logic / flow?

isNone works when you set items as null.
my itemComparator was getting lhs.get('key') === rhs.get('key') problems due to you can't .get on null items.
I removed setting items as null and set them to empty ember objects. If a .get fails on that, at least you get undefined.

sglanzer-deprecated · 2017-03-01T14:02:01Z

addon/utils/selection.js

-    if (isNone(rangeAnchor)) {
+
+    const anchor = items.findIndex(currentItem => itemComparator(currentItem, rangeState['anchor']))
+    // If anchor is -1 then it was a anchor from a previous page that we can not find, so reset


I'm very curious about this comment, can you elaborate on your scenario?

Anchors are being set and assume the 'items' never changes.
When switching pages in pagination, the 20 items that were in 'items' are no longer present.
So looking for anchor in 20 'items' that no longer exist give you -1 in the new 20
'items'. In this case pagination requires you to reset anchors entirely.

sglanzer-deprecated · 2017-03-01T14:02:53Z

addon/utils/selection.js

+
+    const anchor = items.findIndex(currentItem => itemComparator(currentItem, rangeState['anchor']))
+    // If anchor is -1 then it was a anchor from a previous page that we can not find, so reset
+    if (Ember.isEmpty(rangeAnchor) || anchor === -1) {


Destructure isEmpty

Also, the anchor isn't an array, so isNone should be more appropriate

isNone is not good with the new item key.
itemComparators .get method does not work on null items.
isEmpty tests for empty ember objects based what I googled.
It's also passing all the tests in expected behavior so I believe its working as intended.

sglanzer-deprecated · 2017-03-01T14:04:50Z

addon/utils/selection.js

-      // Add the anchor to the selected items
-      selectedItems.pushObject(item)
+      // Add the anchor to the selected items if not already in it from a previous page
+      if (!selectedItems.some(selectedItem => itemComparator(selectedItem, item))) {


Why are we assuming that a comparison needs to be done to check for uniqueness? Do you have duplicate objects/keys in your list?

If you are on page 2 of pagination, switch back to page 1, your previous anchors are no longer valid.
Now that you are on page 1, if you shift select on item already selected, you will just throw it into 'selectedItems'. Since anchors are no longer valid you have no check to see if this item was already selected.

sglanzer-deprecated · 2017-03-01T14:05:30Z

addon/utils/selection.js

@@ -114,6 +116,14 @@ export default {
      }
    }

+    // Wipe out duplicates if range selection covered already selected items
+    // e.g item2-3 already selected but shift click item0 through item5 happens


Again, this behaviour was working previously, what changed?

Visually it works until you count 'selectedItems' growth and you notice duplicates are being sent in by default.

sglanzer-deprecated

I need clarification on what this PR is intended to accomplish and what broke the previously correct behaviour in prior PRs

frosetti · 2017-03-01T18:05:38Z

Some bugs I noticed.

First bug:
itemComparator was getting errors out due to it was receiving null anchor points.

Solution:
don't use null and use empty ember objects and just check if they are empty. This removes .get being used on null items.

Second bug:
When using pagination, switching pages does not reset anchors and trying to find the anchor item from a previous array of items does not work. Use shift click on page 1, switch to page 2 and try to use shift click again. It was not working.

Solution:
When a anchor is -1, it should be safe to assume we are working on a new array of 'items'. That means we should reset the anchors. Anchors from an old array are not useful.

Third bug:
The logic for the range base clicks may not have been correct in the first place (or I broke it) due to 'selectedItems' kept growing.

Why was it growing?:
shift select on item1, pushes item1 in selectedItems (1 item), selectItems.length = 1
shift select on item3, pushes item1-3 in selectedItems (3 item), selectedItems.length = 4
shift select on item5, pushes item1-5 in selectedItems (5 item), selectedItems.length = 9

Fourth bug(I introduced after resetting anchors based on pagination):
If you switch pages and reset anchor, new behaviors need to be addressed.

Scenario 1, two pages, 10 items each page:
page 1, shift click item1, item1 selected
switch to page 2, reset anchors, shift click item 9-10, item 9-10 selected.
switch to page 1, reset anchors, shift click on item 1, ERROR.
Item 1 was already selected on page 1 but now it was throw in selectedItems again due to anchor reset.

Solution:
Be aware of pagination and reset anchors, verify first shift clicked items before sending them into 'selecteditems'

Scenario 2, two pages, 10 items each page:
page 1, shift click item 3-5
switch to page 2, reset anchors, shift click item 9-10
switch to page 1, reset anchors, shift click item 1-6, ERROR
Your shift click now encompassed item 3-5 that was already selected on page one, dupes were thrown into 'selectedItems'.

Solution:
just wipe out dupes that way no crazy logic is needed.

My general opinion is selectedItems should be a map but I was looking to just keep things as they were with out doing major rewrites.

sglanzer-deprecated · 2017-03-02T15:25:08Z

Ok, so if I boil down the issues:

Guard against null during comparison
Support selection reset in pagination
Items added to the selection array aren't being compared correctly, which results in duplicates

Is that a reasonable summary?

frosetti · 2017-03-02T17:37:12Z

Yes, that is what I noticed.

coveralls · 2017-03-02T17:59:22Z

Changes Unknown when pulling 030d36c on frosetti:fr/range_select into ** on ciena-frost:master**.

coveralls · 2017-03-02T18:58:46Z

Changes Unknown when pulling 2333966 on frosetti:fr/range_select into ** on ciena-frost:master**.

coveralls · 2017-03-02T18:58:47Z

Changes Unknown when pulling 2333966 on frosetti:fr/range_select into ** on ciena-frost:master**.

Looks good.

sglanzer-deprecated · 2017-03-03T20:14:20Z

Approved

coveralls · 2017-03-03T20:20:21Z

Changes Unknown when pulling d71a500 on frosetti:fr/range_select into ** on ciena-frost:master**.

sglanzer-deprecated reviewed Mar 1, 2017

View reviewed changes

sglanzer-deprecated suggested changes Mar 1, 2017

View reviewed changes

Update range select

2333966

Steven Glanzer and others added 3 commits March 3, 2017 08:58

Cleanup

ddc7bd6

When selectedItems are not references to items

44e5483

Merge pull request #1 from sglanzer/frosetti/range_select

d71a500

Looks good.

sglanzer-deprecated approved these changes Mar 3, 2017

View reviewed changes

sglanzer-deprecated merged commit 09e99a4 into ciena-frost:master Mar 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fr/range select #113

Fr/range select #113

frosetti commented Mar 1, 2017

sglanzer-deprecated Mar 1, 2017 •

edited

sglanzer-deprecated Mar 1, 2017

frosetti Mar 1, 2017

sglanzer-deprecated Mar 1, 2017

frosetti Mar 1, 2017

sglanzer-deprecated Mar 1, 2017

frosetti Mar 1, 2017

sglanzer-deprecated Mar 1, 2017

frosetti Mar 1, 2017

sglanzer-deprecated Mar 1, 2017

frosetti Mar 1, 2017

sglanzer-deprecated Mar 1, 2017

frosetti Mar 1, 2017

sglanzer-deprecated Mar 1, 2017

sglanzer-deprecated Mar 1, 2017

frosetti Mar 1, 2017

sglanzer-deprecated Mar 1, 2017

frosetti Mar 1, 2017

sglanzer-deprecated Mar 1, 2017

frosetti Mar 1, 2017

sglanzer-deprecated left a comment

frosetti commented Mar 1, 2017 •

edited

sglanzer-deprecated commented Mar 2, 2017

frosetti commented Mar 2, 2017 •

edited

coveralls commented Mar 2, 2017 •

edited

coveralls commented Mar 2, 2017

coveralls commented Mar 2, 2017 •

edited

sglanzer-deprecated commented Mar 3, 2017 •

edited by dafortin

coveralls commented Mar 3, 2017 •

edited

Fr/range select #113

Fr/range select #113

Conversation

frosetti commented Mar 1, 2017

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

CHANGELOG

sglanzer-deprecated Mar 1, 2017 • 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

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

sglanzer-deprecated left a comment

Choose a reason for hiding this comment

frosetti commented Mar 1, 2017 • edited

sglanzer-deprecated commented Mar 2, 2017

frosetti commented Mar 2, 2017 • edited

coveralls commented Mar 2, 2017 • edited

coveralls commented Mar 2, 2017

coveralls commented Mar 2, 2017 • edited

sglanzer-deprecated commented Mar 3, 2017 • edited by dafortin

coveralls commented Mar 3, 2017 • edited

sglanzer-deprecated Mar 1, 2017 •

edited

frosetti commented Mar 1, 2017 •

edited

frosetti commented Mar 2, 2017 •

edited

coveralls commented Mar 2, 2017 •

edited

coveralls commented Mar 2, 2017 •

edited

sglanzer-deprecated commented Mar 3, 2017 •

edited by dafortin

coveralls commented Mar 3, 2017 •

edited