inventory items rank attribut #225

turboMaCk · 2015-12-06T14:48:24Z

Implement rank (enum ['A', 'B', 'C']) on inventory items.

turboMaCk · 2015-12-06T14:51:35Z

app/components/inventory/rank-select.js

@@ -0,0 +1,16 @@
+import Ember from 'ember';
+import SelectValues from 'hospitalrun/utils/select-values';


I've used this util to generate options collection.

turboMaCk · 2015-12-06T14:55:33Z

I see you are using mixins for defining available options / enums for select. My implementation is using wrapper component. I chose this because I want to do it in more component-centric way since there is no need to allow controllers access this options. I'm opened to discussion on this decision.

stefanpenner · 2015-12-06T14:57:09Z

app/components/inventory/rank-select.js

+
+export default Ember.Component.extend({
+  label: 'Rank',
+  rankOptions: ['A', 'B', 'C'],


lets put this in init or a CP, putting complex values on the prototype is likely to cause grief.

actual options (Array of Objects) is defined via CP in here. I can define setter for this in init() if you like to.

turboMaCk · 2015-12-06T16:26:08Z

Design

This is how it looks like in lists:

this is edit form:

Ideas on improvements:

We can add binding on item.quantity to inventory/rank-display component and compute classes (danger/success...) based on quantity and rank. But I'm nut sure if this should also depend on Distribution Unit type, so I'm leaving it as it is since this needs definition.

Let me know what you think about this PR. I'm ready to work on improvements based on your feedback.

tangollama · 2015-12-06T18:03:23Z

@turboMaCk the screenshots look great. I'll review w/ @jkleinsc tomorrow and either accept the PR or get you feedback. Thanks for hopping right in.

stefanpenner · 2015-12-06T20:12:18Z

app/components/inventory/rank-select.js

+
+export default Ember.Component.extend({
+  label: 'Rank',
+  rankOptions: [],


this looks like instance state, it should likely be instantiated in the initializer, or as a CP.

Placing instance state of the prototype can often lead to unexpected behavior and leaks).

If you want the slot on the prototype for documentation, let my suggestion making the value undefined or null. Alliteratively it could be a CP, which insures the per instance state.

rankOptions: Ember.computed(function() { return ['A', 'B', 'C']; });

In ES7 we will get property initializers, which make this nicer and less confusing.

class Foo { rankOptions = ['A', 'B', 'C']; // re-rerun for each instantiation, and set to `this` }

Imho setting this in initializer is little bit over-kill and made code confusing. Setting this to undefined/null by default do not specify type (maybe it will be better to remove this and keep only setter in init). The only reason I kept this is to let everyone know that this property will be and it should be Array. Computed property seems to be best solution but I'm not sure if there is no any reasonable overhead (since this type implements little bit more than we actually need). ES7 solution seems to be best for me, but I'm not sure, if it's compatible.

I can remove this completely (but for me it makes sense to have defined all props that instance has on one place) and keep the setter in init() or make CP (if it is not too much expensive)

I do not know as much about ember-metal as you so let me know your suggestions on this.

BTW... this discussion is still actual even github hides this.

jglovier · 2015-12-07T16:04:00Z

@turboMaCk thanks for your work here. Some design feedback:

These labels should use colors from the existing color palette here. Although I looked through the code and it appears you're adding classes, but not defining colors anywhere? Now I'm wondering where those colors are coming from...

As for the edit screen, the only thing that concerns me is the Purchases table heading looks really crammed. It would be great to either reduce the label copy a bit (i.e. drop words where appropriate, like "Date" when date is obvious from the value), or find a way to better accommodate everything without it having to wrap to two lines of text everywhere.

jkleinsc · 2015-12-07T17:05:44Z

@jglovier Those colors I believe are coming from bootstrap. Also, the purchases table heading shown here has nothing to do with this PR.

@turboMaCk I am thinking we shouldn't display the ranking on the item listing. I like what you've done there, but it's information that really is only needed for reporting and/or alerting purposes, so I don't think it belongs on the listing screen.

jglovier · 2015-12-07T17:10:33Z

Those colors I believe are coming from bootstrap

Ahhh, right.

Also, the purchases table heading shown here has nothing to do with this PR.

Okay, cool. I'll address that separately then.

turboMaCk · 2015-12-07T17:14:06Z

@jglovier is completely right. I used bootstrap's labels for this. The only thing I had to change in form is name input and that rank select. I made a name input smaller.

We have to decide if we want to show that rank column in list tables. It also could be done some other way like I mentioned before it at least makes sense to me to add some running out of icon in the future.

I also want to continue and work on this #209 you after we finish this if want to.

turboMaCk · 2015-12-07T17:21:11Z

If I should say my opinion on displaying this thing in list I will go this way:

keep displaying this information colorised in list till alert system will be done.
after alert system is ready change this to displaying something like condition based on rank and quantity.

jkleinsc · 2015-12-07T17:46:29Z

@tangollama Any thoughts on this?

turboMaCk · 2015-12-08T19:51:15Z

I will probably find some time to continue on this tomorrow. Let me know about changes 🚀

jkleinsc · 2015-12-09T15:35:26Z

@tangollama ping
I am of the opinion we do not include the rank on the inventory listing screen, primarily because there are a decent number of fields being displayed currently. Also, if we are looking for a visual indicator, that could be applied to the quantity column, but I think that should be handled as a separate issue/PR

turboMaCk · 2015-12-09T16:03:28Z

@jkleinsc so for now that setting will just do nothing. Ok, I do not thing that this is big deal so there is no need to sped much more time on this decision. I'll update this PR. and then ping for code review.

turboMaCk · 2015-12-09T18:48:56Z

READY.

I also move component files to pod structure. I do not know why ember g component do not generate this by default.

turboMaCk · 2015-12-10T16:10:49Z

rebased

turboMaCk · 2015-12-10T16:15:51Z

app/inventory/rank-select/component.js

+    this._super(...arguments);
+
+    // set available options
+    this.set('rankOptions', Ember.A(['A', 'B', 'C']));


because of this options can not be overwrite. Also there is unfinished discussion with @stefanpenner

#225 (diff)
#225 (comment)

turboMaCk · 2015-12-10T16:24:36Z

not ok 21 PhantomJS - Browser "phantomjs /home/travis/build/HospitalRun/hospitalrun-frontend/node_modules/ember-cli/node_modules/testem/assets/phantom.js http://localhost:7357/5350" exited unexpectedly.

EDIT fixed on rebuid.

jkleinsc · 2015-12-10T16:59:43Z

@turboMaCk Not sure why PhantomJS died -- maybe just a problem in Travis. I re-ran the job and it looks good now. As far as the options are concerned, one pattern we have used is to use mixins for hardcoded options. If we ever want to change the options to pull from the database instead, we just change the mixin.

turboMaCk · 2015-12-10T17:04:02Z

@jkleinsc thank you!

I know about mixins #225 (comment) I just waited for your feedback. Personally I like using components more than mixins in controllers (controllers are singletons & it's better isolated from other stuff) I can move this from component to mixin and use that mixin in that component but then there was no much other logic in component itself:D

Let me know what you think. I'll do this your way.

jkleinsc · 2015-12-10T17:16:52Z

@turboMaCk I missed the comment about mixins earlier. I think for now we should just leave it the way you coded it. I will merge the PR unless you think there is another reason to wait.

turboMaCk · 2015-12-10T17:18:58Z

I do not think that there are any blocker. I'll look at this after merge: #209

inventory items rank attribute

jkleinsc · 2015-12-10T17:52:00Z

Looks good @turboMaCk. Thanks for the PR!

turboMaCk · 2015-12-10T17:57:13Z

My pleasure to helping you guys with this amazing project.

refactor(NewLabRequest): improve queries - remove function call tests

turboMaCk mentioned this pull request Dec 6, 2015

Add an A,B,C ranking to inventory items #207

Closed

turboMaCk reviewed Dec 6, 2015
View reviewed changes

stefanpenner reviewed Dec 6, 2015
View reviewed changes

turboMaCk changed the title ~~inventory/rank-select component~~ inventory items rank attribut Dec 6, 2015

turboMaCk force-pushed the feature/inventory_items_rank branch from bf5f068 to 6edc204 Compare December 6, 2015 16:14

stefanpenner reviewed Dec 6, 2015
View reviewed changes

turboMaCk force-pushed the feature/inventory_items_rank branch from 40ded82 to d22287c Compare December 10, 2015 16:10

turboMaCk added 2 commits December 10, 2015 17:12

inventory/rank-select component

4e9933e

rank-select component pod structure

e202ae9

turboMaCk force-pushed the feature/inventory_items_rank branch from d22287c to e202ae9 Compare December 10, 2015 16:13

turboMaCk reviewed Dec 10, 2015
View reviewed changes

jkleinsc added a commit that referenced this pull request Dec 10, 2015

Merge pull request #225 from turboMaCk/feature/inventory_items_rank

1c6dc42

inventory items rank attribute

jkleinsc merged commit 1c6dc42 into HospitalRun:master Dec 10, 2015

matteovivona pushed a commit that referenced this pull request Jan 15, 2021

Merge pull request #225 from codyarose/codyarose_NewLabRequest

ac35b21

refactor(NewLabRequest): improve queries - remove function call tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inventory items rank attribut #225

inventory items rank attribut #225

turboMaCk commented Dec 6, 2015

turboMaCk Dec 6, 2015

turboMaCk commented Dec 6, 2015

stefanpenner Dec 6, 2015

turboMaCk Dec 6, 2015

turboMaCk commented Dec 6, 2015

tangollama commented Dec 6, 2015

stefanpenner Dec 6, 2015

turboMaCk Dec 6, 2015

turboMaCk Dec 6, 2015

jglovier commented Dec 7, 2015

jkleinsc commented Dec 7, 2015

jglovier commented Dec 7, 2015

turboMaCk commented Dec 7, 2015

turboMaCk commented Dec 7, 2015

jkleinsc commented Dec 7, 2015

turboMaCk commented Dec 8, 2015

jkleinsc commented Dec 9, 2015

turboMaCk commented Dec 9, 2015

turboMaCk commented Dec 9, 2015

turboMaCk commented Dec 10, 2015

turboMaCk Dec 10, 2015

turboMaCk commented Dec 10, 2015

jkleinsc commented Dec 10, 2015

turboMaCk commented Dec 10, 2015

jkleinsc commented Dec 10, 2015

turboMaCk commented Dec 10, 2015

jkleinsc commented Dec 10, 2015

turboMaCk commented Dec 10, 2015

		@@ -0,0 +1,16 @@
		import Ember from 'ember';
		import SelectValues from 'hospitalrun/utils/select-values';

inventory items rank attribut #225

inventory items rank attribut #225

Conversation

turboMaCk commented Dec 6, 2015

turboMaCk Dec 6, 2015

Choose a reason for hiding this comment

turboMaCk commented Dec 6, 2015

stefanpenner Dec 6, 2015

Choose a reason for hiding this comment

turboMaCk Dec 6, 2015

Choose a reason for hiding this comment

turboMaCk commented Dec 6, 2015

Design

Ideas on improvements:

tangollama commented Dec 6, 2015

stefanpenner Dec 6, 2015

Choose a reason for hiding this comment

turboMaCk Dec 6, 2015

Choose a reason for hiding this comment

turboMaCk Dec 6, 2015

Choose a reason for hiding this comment

jglovier commented Dec 7, 2015

jkleinsc commented Dec 7, 2015

jglovier commented Dec 7, 2015

turboMaCk commented Dec 7, 2015

turboMaCk commented Dec 7, 2015

jkleinsc commented Dec 7, 2015

turboMaCk commented Dec 8, 2015

jkleinsc commented Dec 9, 2015

turboMaCk commented Dec 9, 2015

turboMaCk commented Dec 9, 2015

turboMaCk commented Dec 10, 2015

turboMaCk Dec 10, 2015

Choose a reason for hiding this comment

turboMaCk commented Dec 10, 2015

jkleinsc commented Dec 10, 2015

turboMaCk commented Dec 10, 2015

jkleinsc commented Dec 10, 2015

turboMaCk commented Dec 10, 2015

jkleinsc commented Dec 10, 2015

turboMaCk commented Dec 10, 2015