Add primitives for frost-list-item #74

quincyle · 2016-11-28T06:09:15Z

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 frost-list-item-badge-count component.
Added frost-list-item-boolean component.
Added frost-list-item-date component.
Added frost-list-item-icon-text component.
Added frost-list-item-large-icon-text component.
Added frost-list-item-primary-identifier component.
Added frost-list-item-secondary-identifier component.
Added frost-list-item-unlabeled-property component.
Added frost-list-item-small-view component.
Added frost-list-item-primary-state component.

quincyle · 2016-11-28T06:16:04Z

coveralls · 2016-11-28T06:19:47Z

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

coveralls · 2016-11-28T06:19:47Z

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

coveralls · 2016-11-28T06:36:57Z

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

coveralls · 2016-11-28T06:36:57Z

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

coveralls · 2016-11-28T06:50:21Z

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

coveralls · 2016-11-28T06:50:21Z

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

dafortin

Are we planning to add "hooks"?
Are we planning to add more tests eventually?

dafortin · 2016-11-28T12:36:49Z

addon/components/primitives/frost-list-item-end-point.js

+import Ember from 'ember'
+import layout from '../../templates/components/frost-list-item-end-point'
+
+export default Ember.Component.extend({


Should we get a default class here?

dafortin · 2016-11-28T12:37:54Z

addon/components/primitives/frost-list-item-end-point.js

@@ -0,0 +1,6 @@
+import Ember from 'ember'


Endpoint is a very "ciena" concept should we really have something like this in a "generic" repo?

No, this is a miss and can be removed

dafortin · 2016-11-28T12:39:37Z

addon/components/primitives/frost-list-item-large-view.js

+import Ember from 'ember'
+import layout from '../../templates/components/frost-list-item-large-view'
+
+export default Ember.Component.extend({


Should we have a default class?

dafortin · 2016-11-28T12:41:08Z

addon/styles/_list-item-primitives.scss

@@ -0,0 +1,185 @@
+@import 'node_modules/ember-frost-core/addon/styles/frost-theme';
+
+$frost-list-item-primary-state-critical: #E52C2C;


Do you have sass linting enable? I think sass linting wants this to be in small case.

Are you sure? I would have thought that upper case was the standard

dafortin · 2016-11-28T12:46:38Z

Approved since there is only very small things to change/questions.

sglanzer-deprecated · 2016-11-28T15:49:28Z

Looks like a few of the icon/text positions are slightly off (boolean, badge count)

sglanzer-deprecated · 2016-11-28T15:51:29Z

addon/components/primitives/frost-list-item-large-icon-text.js

@@ -0,0 +1,22 @@
+import Ember from 'ember'


Can this component just be done using a class modifier on the icon-text primitive?

sglanzer-deprecated · 2016-11-28T15:51:48Z

addon/components/primitives/frost-list-item-large-view.js

@@ -0,0 +1,6 @@
+import Ember from 'ember'


Not sure what this component is

sglanzer-deprecated · 2016-11-28T15:52:02Z

addon/components/primitives/frost-list-item-primary-identifier.js

+  classNames: ['frost-list-item-primary-identifier'],
+
+  propTypes: {
+    value: PropTypes.string


sglanzer-deprecated

Let's get a 30-60 minute discussion scheduled on this with the same group as the object browser review

sglanzer-deprecated · 2016-11-28T15:54:37Z

addon/components/primitives/frost-list-item-primary-state.js

+    }
+  },
+
+  layout


We have some new conventions around comment headers that I think we should put in place here. This is an example https://github.com/ciena-frost/ember-frost-list/blob/master/addon/components/frost-pagination.js

sglanzer-deprecated · 2016-11-28T15:55:13Z

addon/components/primitives/frost-list-item-small-view.js

@@ -0,0 +1,16 @@
+import Ember from 'ember'


Is this component a thing anymore?

sglanzer-deprecated · 2016-11-28T15:55:47Z

addon/components/primitives/frost-list-item-unlabeled-property.js

@@ -0,0 +1,19 @@
+import Ember from 'ember'


I think we could genericize this as property and just react to the presence/absence of a label

sglanzer-deprecated · 2016-11-28T15:56:49Z

addon/styles/_list-item-primitives.scss

@@ -0,0 +1,185 @@
+@import 'node_modules/ember-frost-core/addon/styles/frost-theme';
+
+$frost-list-item-primary-state-critical: #E52C2C;


Are you sure? I would have thought that upper case was the standard

sglanzer-deprecated · 2016-11-28T15:57:57Z

addon/styles/_list-item-primitives.scss

+  flex-direction: row;
+  align-items: center;
+  height: 30px;
+  .frost-list-item-boolean-label {


No need to nest if you're using an full name and not using &- (we should probably discuss the convention there before we proceed)

sglanzer-deprecated · 2016-11-28T16:00:06Z

addon/styles/_list-item-primitives.scss

+  }
+}
+
+.frost-list-item-small-view {


Ahhh, now I get where the component was coming from before...this is interesting. If we talk about the structure of the "header"/"small" view of a list item then there are definitely conventions that could be put in place, but it would become more restrictive I believe - let's chat about this with a group - maybe we can schedule 30 minutes or so with the same group reviewing the object browser?

sglanzer-deprecated · 2016-11-28T16:01:49Z

tests/dummy/app/pods/item-primitives/controller.js

+export default Ember.Controller.extend({
+  smallViewData: {
+    'label-name': 'EBUTORO000034EBUTORO000034EBUTORO000034EBUTORO000034EBUTORO000034EBUTORO000034',
+    label: 'EBUTORO......000034',


Inserting the ellipsis ourselves is deceiving...I wouldn't do that

sglanzer-deprecated · 2016-11-28T16:02:52Z

tests/dummy/app/pods/item-primitives/template.hbs

@@ -0,0 +1,155 @@
+<div style="margin: 30px">


I find it interesting that you've specified the small/large view in components, but aren't using them here - we can follow up on this after the 30 minute meeting

sglanzer-deprecated · 2016-11-28T16:04:00Z

tests/dummy/app/styles/app.scss

@@ -176,4 +176,34 @@
  }
 }

+.frost-list-demo-primitives {


I don't want to see anything used in the list item coming from the dummy - if there is value in flex primitives then it should be part of the consumable interface

quincyle added 4 commits November 27, 2016 12:54

add styles for primitives && add new route

25a01da

merge remote master

398f514

add primitives for frost-list-item

c7c79e1

add item-primitives route for demo && update relative demo styles

8a4e23f

quincyle added the bug label Nov 28, 2016

quincyle assigned rox163, sglanzer-deprecated and dafortin Nov 28, 2016

quincyle added enhancement and removed bug labels Nov 28, 2016

update styles for frost-list-item-boolean

86ece56

cleanup test property

c726217

dafortin approved these changes Nov 28, 2016

View reviewed changes

sglanzer-deprecated reviewed Nov 28, 2016

View reviewed changes

sglanzer-deprecated suggested changes Nov 28, 2016

View reviewed changes

rox163 removed their assignment Oct 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add primitives for frost-list-item #74

Add primitives for frost-list-item #74

quincyle commented Nov 28, 2016 •

edited

Loading

quincyle commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

dafortin left a comment

dafortin Nov 28, 2016

dafortin Nov 28, 2016

sglanzer-deprecated Nov 28, 2016

dafortin Nov 28, 2016

dafortin Nov 28, 2016

sglanzer-deprecated Nov 28, 2016

dafortin commented Nov 28, 2016

sglanzer-deprecated commented Nov 28, 2016

sglanzer-deprecated Nov 28, 2016

sglanzer-deprecated Nov 28, 2016

sglanzer-deprecated Nov 28, 2016

sglanzer-deprecated left a comment

sglanzer-deprecated Nov 28, 2016

sglanzer-deprecated Nov 28, 2016

sglanzer-deprecated Nov 28, 2016

sglanzer-deprecated Nov 28, 2016

sglanzer-deprecated Nov 28, 2016

sglanzer-deprecated Nov 28, 2016

sglanzer-deprecated Nov 28, 2016

sglanzer-deprecated Nov 28, 2016

sglanzer-deprecated Nov 28, 2016

		@@ -0,0 +1,185 @@
		@import 'node_modules/ember-frost-core/addon/styles/frost-theme';

		$frost-list-item-primary-state-critical: #E52C2C;

+                  }
+                },
+                layout

Add primitives for frost-list-item #74

Are you sure you want to change the base?

Add primitives for frost-list-item #74

Conversation

quincyle commented Nov 28, 2016 • edited Loading

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

CHANGELOG

quincyle commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

coveralls commented Nov 28, 2016

dafortin 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

dafortin commented Nov 28, 2016

sglanzer-deprecated commented Nov 28, 2016

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

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

quincyle commented Nov 28, 2016 •

edited

Loading