Internal update to fit ember-frost-object-browser #69

quincyle · 2016-11-09T07:20:27Z

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

Upgraded ember to 2.8
Removed sortedItems property from sort Mixin.
Removed filteredItems property from core Mixin.
Removed defaultHeight property in frost-list-core component.
Redesigned sort Mixin.
Updated dummy app based on addon changes.
Updated dependencies.

into lts-rework-ob

coveralls · 2016-11-09T07:29:21Z

Changes Unknown when pulling 0c8a24d on quincyle:lts-rework-ob into * on ciena-frost:master*.

sglanzer-deprecated

I need some context on a few of the decisions (defaultHeight, sortItems action behaviour)

sglanzer-deprecated · 2016-11-09T12:47:25Z

addon/components/frost-list-core.js

@@ -39,7 +36,6 @@ const FrostList = Component.extend(PropTypeMixin, {
  },

  alwaysUseDefaultHeight: false,
-  defaultHeight: 45,


Is this no longer necessary (and why)? Also, if it's still a useful mechanism, can it be set from the component interface?

I put it here original to fix a s&m regression, and now it looks ok without it after I update the s&m from 0.4.x to 0.5.x, instead it produces a wired css issue. There's still a chain in template that user can set defaultHeightvia component interface.

sglanzer-deprecated · 2016-11-09T12:48:40Z

addon/mixins/frost-list-mixin.js

@@ -44,12 +44,12 @@ export default Mixin.create(FrostListSelectionMixin, FrostListExpansionMixin, Fr
    )
  }),

-  listMixinConfig: Ember.computed('activeSorting', 'sortableProperties', 'sortedItems.[]', function () {
+  listMixinConfig: Ember.computed('activeSorting', 'sortableProperties', 'statefulListItems.[]', function () {
    let activeSorting = this.get('activeSorting')


These lets aren't necessary, the get can be put directly into the return object

sglanzer-deprecated · 2016-11-09T12:51:32Z

addon/mixins/frost-list-sorting-mixin.js

-      this.set('activeSorting', activeSorting)
+      let normalizedSortProperties = normalizeSort(filteredSortProperties)
+      const dataKey = this.get('listConfig.items')
+      this.set(dataKey, listDefaultSort(this.get(dataKey), normalizedSortProperties))


Shouldn't this be conditional on someone providing their own sort function? I.E. can you override the default or not sort on the client at all?

Currently, I'm assuming consumer will replace the sortItems function entirely if they don't want the default sort or client sort. I can make an interface that takes normalizeSort out and let user only overwrites what's left over.

sglanzer-deprecated · 2016-11-09T12:52:54Z

addon/templates/frost-list-core.hbs

-        {{/if}}
+        {{component item
+          model=record
+          hook=(concat hook '-item-' index)


If you have a hookQualifier below with the index, why concat the index on the hook?

When I was working on the like, the hoookQualifier is already a thing but not working properly yet.

Now you have both :) Maybe remove the concat at this stage?

sglanzer-deprecated · 2016-11-09T12:53:49Z

addon/utils/utils.js

+    return []
+  }
+  return sort.map(function (item) {
+    let key = item.value


const or just move it into the string literal

sglanzer-deprecated · 2016-11-09T12:56:29Z

addon/utils/utils.js

+  })
+}
+
+export function listDefaultSort (items, sortProperties) {


No need to prefix this with list and you should add a jsdoc here or in the README explaining that you're using Ember.compare and link to the documentation http://emberjs.com/api/classes/Ember.Comparable.html#method_compare

sglanzer-deprecated · 2016-11-09T12:57:51Z

addon/utils/utils.js

+    return resultArray
+  })
+
+  return Ember.A(items.slice().sort((itemA, itemB) => {


Add a comment here explaining that slice is used to create a copy of the array so that the sort isn't applied to the original

rox163 · 2016-11-09T16:59:05Z

package.json

-    "ember-browserify": "1.1.12",
-    "ember-cli": "^2.7.0",
+    "ember-browserify": "1.1.13",
+    "ember-cli": "^2.8.0",


~2.8.0 please. No ^

rox163 · 2016-11-09T16:59:19Z

package.json

-    "ember-concurrency": "0.7.8",
-    "ember-data": "^2.4.0",
+    "ember-concurrency": "0.7.15",
+    "ember-data": "^2.8.0",


no ^ please

rox163 · 2016-11-09T16:59:38Z

package.json

-    "ember-frost-sort": "^3.0.0",
-    "ember-get-config": "0.1.7",
+    "ember-frost-core": "0.29.1",
+    "ember-frost-sort": "5.0.1",


With ^ please

rox163 · 2016-11-09T16:59:45Z

package.json

    "ember-hash-helper-polyfill": "0.1.1",
-    "ember-hook": "1.3.2",
+    "ember-hook": "1.3.5",


with ^ please

rox163 · 2016-11-09T16:59:54Z

package.json

-    "ember-prop-types": "^2.1.0",
-    "ember-redux": "1.5.3",
+    "ember-one-way-controls": "1.1.1",
+    "ember-prop-types": "3.0.2",


rox163 · 2016-11-09T17:00:53Z

package.json

@@ -26,41 +26,43 @@
  "devDependencies": {


I cant directly comment on it but please change line 18 node version to >= 6.0.0

rox163

With the upgrade to 2.8 we also need a small change in .travis.yml. Remove lines 1-5 and replace with this block -

sudo: required
dist: trusty
language: node_js
node_js:
  - '6.9'
  - 'stable'

This would come before the branches statement.
Thanks.

dafortin · 2016-11-09T17:07:04Z

We will also need to move to ember-cli-code-coverage@0.3.5 (can be done in the future)
Run test on chrome (change testem and .travis.yml)

coveralls · 2016-11-10T06:04:48Z

Changes Unknown when pulling 33d920f on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-10T06:04:48Z

Changes Unknown when pulling 33d920f on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-10T07:07:02Z

Changes Unknown when pulling b80e693 on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-10T07:07:02Z

Changes Unknown when pulling b80e693 on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-10T07:17:37Z

Changes Unknown when pulling 2b1249d on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-10T07:17:37Z

Changes Unknown when pulling 2b1249d on quincyle:lts-rework-ob into * on ciena-frost:master*.

rox163 · 2016-11-10T16:19:10Z

addon/styles/addon.scss

@@ -56,7 +56,7 @@ $frost-list-content-border-bottom: 1px;
      margin-right: 15px;
      background-color: $frost-color-lgrey-1;
    }
-    .container {
+    .-container {


is this supposed to be &-container?

coveralls · 2016-11-10T19:06:12Z

Changes Unknown when pulling 6de98ca on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-10T19:06:12Z

Changes Unknown when pulling 6de98ca on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-10T19:29:36Z

Changes Unknown when pulling c9202eb on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-10T19:29:36Z

Changes Unknown when pulling c9202eb on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-10T19:45:14Z

Changes Unknown when pulling 62722b2 on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-10T19:45:14Z

Changes Unknown when pulling 62722b2 on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-10T19:50:49Z

Changes Unknown when pulling 62722b2 on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-10T19:58:36Z

Changes Unknown when pulling e4a4618 on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-10T19:58:36Z

Changes Unknown when pulling e4a4618 on quincyle:lts-rework-ob into * on ciena-frost:master*.

sglanzer-deprecated

Minor stuff on my end, this looks good overall

sglanzer-deprecated · 2016-11-11T14:45:06Z

addon/mixins/frost-list-sorting-mixin.js

 } = Ember
-import computed from 'ember-computed-decorators'
+import {normalizeSort, defaultSort} from 'ember-frost-list/utils/utils'


Can we re-export these utils from the index.js and avoid the path? If they aren't intended to be accessed externally then you can ignore this.

I might also suggest that instead of one generic utils module you would call this utils/sort since it contains sort utils

sglanzer-deprecated · 2016-11-11T14:45:33Z

addon/mixins/frost-list-sorting-mixin.js

 import FrostListCoreMixin from 'ember-frost-list/mixins/frost-list-core-mixin'

 export default Mixin.create(FrostListCoreMixin, {
  // == Event =================================================================
+  // TODO replace defineProperty when there's a public method available


Raise this as an issue, you can remove the TODO

sglanzer-deprecated · 2016-11-11T14:47:12Z

addon/mixins/frost-list-sorting-mixin.js

+    sortItems (sortProperties) {
+      const normalizedSortProperties = normalizeSort(sortProperties)
+      const customSortMethod = get(this, 'listConfig.sorting.client')
+      const sortMethod = typeof customSortMethod === 'function' ? customSortMethod : defaultSort


Are we using prop-types for the sorting.client type? If so, we don't need this check (if not, we probably should use prop-types)

Basically I'm looking for an assertion instead of a silent fallback

sglanzer-deprecated · 2016-11-11T14:49:21Z

addon/templates/frost-list-core.hbs

-        {{/if}}
+        {{component item
+          model=record
+          hook=(concat hook '-item-' index)


Now you have both :) Maybe remove the concat at this stage?

sglanzer-deprecated · 2016-11-11T14:54:18Z

addon/utils/utils.js

+
+  // slice is used to create a copy of the array so that the sort isn't applied to the original data in store
+  return Ember.A(items.slice().sort((itemA, itemB) => {
+    for (let i = 0; i < normalizedSortProperties.length; i++) {


Any reason to avoid a forEach?

sglanzer-deprecated · 2016-11-11T16:40:24Z

Approved

coveralls · 2016-11-11T16:43:22Z

Changes Unknown when pulling a9b15a8 on quincyle:lts-rework-ob into * on ciena-frost:master*.

coveralls · 2016-11-11T16:43:22Z

Changes Unknown when pulling a9b15a8 on quincyle:lts-rework-ob into * on ciena-frost:master*.

quincyle added 9 commits October 6, 2016 22:49

change cp chain

5a0fc9e

update dummy app

ab90b89

updated mixins to work with object browser

892b933

fix default height issue

56863f2

Merge branch 'master' of https://github.com/ciena-frost/ember-frost-list

873712e

into lts-rework-ob

add .github

204bcfa

update addon and demo app

02025a5

update package.json and bower.json

9c1eff7

update dependencies

0c8a24d

quincyle added the enhancement label Nov 9, 2016

sglanzer-deprecated suggested changes Nov 9, 2016

View reviewed changes

rox163 reviewed Nov 9, 2016

View reviewed changes

package.json

"ember-hash-helper-polyfill": "0.1.1",

"ember-hook": "1.3.2",

"ember-hook": "1.3.5",

Copy link

Contributor

rox163 Nov 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with ^ please

rox163 reviewed Nov 9, 2016

View reviewed changes

rox163 suggested changes Nov 9, 2016

View reviewed changes

update travis and dependencies

33d920f

update based on comments

b80e693

minor css update

2b1249d

replace defineProperty with set whereever its possible

7b070ba

change ci to run on Chrome

ad3b3f8

rox163 reviewed Nov 10, 2016

View reviewed changes

rox163 approved these changes Nov 10, 2016

View reviewed changes

quincyle added 3 commits November 10, 2016 12:06

update travis.yml

2e604ca

swtich from object.get(prop) to Ember.get(object, prop)

5ecba37

fix linting

6de98ca

quincyle added 2 commits November 10, 2016 14:12

fix style issue for frost-list-content-container

8e9b87c

upate styles

c9202eb

quincyle added 2 commits November 10, 2016 14:33

fix regression

62722b2

minor style change.

e4a4618

sglanzer-deprecated approved these changes Nov 11, 2016

View reviewed changes

update based on comments

a9b15a8

quincyle merged commit 9781cca into ciena-frost:master Nov 11, 2016

Internal update to fit ember-frost-object-browser #69

Internal update to fit ember-frost-object-browser #69

Conversation

quincyle commented Nov 9, 2016

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

CHANGELOG

coveralls commented Nov 9, 2016 • edited Loading

sglanzer-deprecated 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

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

rox163 left a comment

Choose a reason for hiding this comment

dafortin commented Nov 9, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

Choose a reason for hiding this comment

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016 • edited Loading

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

sglanzer-deprecated 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

sglanzer-deprecated commented Nov 11, 2016 • edited by job13er Loading

coveralls commented Nov 11, 2016

coveralls commented Nov 11, 2016

coveralls commented Nov 9, 2016 •

edited

Loading

coveralls commented Nov 10, 2016 •

edited

Loading

sglanzer-deprecated commented Nov 11, 2016 •

edited by job13er

Loading