openlmis-stockmanagement-ui / src / stockmanagement

stockmangement.scss

Clone Tools

Stats

Commits this week: 0
Total Committers: 3
Last Commit: 18 Jul 17
Commits this week: 0

Lines of code count not available

Commit Activity

Commits By Day In Week
52 week commits volume
Commits By Day In Week
Commits by day
Commits By Day In Week
Commits by hour
Commits By Hour In Day
Commit calendar
 

Most active committers (90 days)

loading
loading
  • last updated a few minutes ago
All Commits Reviews
Constraints
Constraints: committers
 
Constraints: files
Constraints: dates
Monday 31 Jul 2017
8:57 am

Nikodem Graczewski

Friday 21 Jul 2017
2:32 pm

Nikodem Graczewski

Created a separate review for this - FEOLMIS-1727.

Created a separate review for this - FEOLMIS-1727.

12:53 pm

Mateusz Kwiatkowski

12:52 pm

Mateusz Kwiatkowski

Move all property assignments to onInit method

Move all property assignments to onInit method

Thursday 20 Jul 2017
8:04 pm

Nick Reid

Tuesday 18 Jul 2017
11:40 pm

Nick Reid <nickreid@nickreid.com>

OLMIS-2797: Removed CSS that was overly general and repetative

Options
    • -29
    • +0
    ./stockmangement.scss
11:30 am

Nikodem Graczewski

8:57 am

Nikodem Graczewski

Do we want to make the method take a list of arguments(title, message etc.) instead of an object?

Do we want to make the method take a list of arguments(title, message etc.) instead of an object?

8:54 am

Nikodem Graczewski

Yes, sorry. The other one is for a separate branch.

Yes, sorry. The other one is for a separate branch.

8:53 am

Nikodem Graczewski

I agree.

I agree.

Monday 17 Jul 2017
6:02 pm

Nick Reid

Overall, this looks good — I don't love the preSave/postSave hooks being so explicit — I feel like we could have a better interface if we moved more logic into the adjustments-modal.service.js

Overall, this looks good — I don't love the preSave/postSave hooks being so explicit — I feel like we could have a better interface if we moved more logic into the adjustments-modal.service.js

5:59 pm

Nick Reid

This should should be prefixed somewhere, so the change stays contained to this specific area...

This should should be prefixed somewhere, so the change stays contained to this specific area...

5:56 pm

Nick Reid

If possible — delete this entire file, its a great example of duplicative CSS

If possible — delete this entire file, its a great example of duplicative CSS

5:54 pm

Nick Reid

Like I noted in the other review — setting and copying options variables would be more clear here — and also make the contract of how to use this modal more obvious

Like I noted in the other review — setting and copying options variables would be more clear here — and also make the contract of how to use this modal more obvious

5:52 pm

Nick Reid

+1 – unit tests for this file will also help make its actions clear (IE you get list back with changes)

+1 – unit tests for this file will also help make its actions clear (IE you get list back with changes)

5:50 pm

Nick Reid

I think we have two reviews for this same ticket — crap — please remove this file if we are not using it...

I think we have two reviews for this same ticket — crap — please remove this file if we are not using it...

5:50 pm

Nick Reid

I'm not loving pre-cancel or pre-save calls — I see that we need it to provide extra validation, and ideally before the controller's scope is destroyed... which seems to be the reason we don't use ...

I'm not loving pre-cancel or pre-save calls — I see that we need it to provide extra validation, and ideally before the controller's scope is destroyed... which seems to be the reason we don't use a promise and just "re-open" the modal

This is totally fine for now — we might want to explore adding an event here // or finding a way to use a promise, where closing/destroying the scope happens after the code written in the implementing controller.

Be sure to document this interface

5:32 pm

Nick Reid

+1

+1

3:49 pm

Nick Reid

3:49 pm

Nick Reid

This looks great — there are a couple comments and "unresolved" that I think can be optional

This looks great — there are a couple comments and "unresolved" that I think can be optional

3:48 pm

Nick Reid

Cool — I'll try to see where else this happens and make a refactor ticket about it (which we can do.... later)

Cool — I'll try to see where else this happens and make a refactor ticket about it (which we can do.... later)

3:47 pm

Nick Reid

that is such a tiny dumb difference — but probably not worthing making some abstraction to deal with — and let services do their own thing

that is such a tiny dumb difference — but probably not worthing making some abstraction to deal with — and let services do their own thing

3:45 pm

Nick Reid

I like the use of $onInit and $element — it looks like we almost don't use $scope.... Overall this will become more testable and extendable — which we should document and recommend (eventually)

I like the use of $onInit and $element — it looks like we almost don't use $scope.... Overall this will become more testable and extendable — which we should document and recommend (eventually)

3:44 pm

Nick Reid

This looks like the only function that uses $scope, and it seems like this check could happen more cleanly somewhere else. This is also something we could clean up later

This looks like the only function that uses $scope, and it seems like this check could happen more cleanly somewhere else.

This is also something we could clean up later

3:25 pm

Nick Reid

Looks good — not 100% sure we need the "digits-cell" class — but its good enough for now

Looks good — not 100% sure we need the "digits-cell" class — but its good enough for now

3:19 pm

Nick Reid

Question: Can we expect .stockAjustments to be null? (or false, "2", or some other value) This seems like a great place to set a default value

Question: Can we expect .stockAjustments to be null? (or false, "2", or some other value)

This seems like a great place to set a default value

3:08 pm

Nick Reid

(1) Yes, that's fugly. (2) I really do think this component should be in referencedata-ui — it uses base components to provide logic — and has an idea of data specifically from OpenLMIS (adjustmen...

(1) Yes, that's fugly.

(2) I really do think this component should be in referencedata-ui — it uses base components to provide logic — and has an idea of data specifically from OpenLMIS (adjustments)

I'd just do simple things like:

  • Set adjustments to an empty array (if it isn't)
  • Set reasons to an empty array (if it isn't)
  • and specify options as false at the top of the file so its really clear whats in the options object.


var defaultOptions = {
  adjustments: [],
  reasons: [],
  message: false,
  title: messageService.get("default title")
};

options = angular.extend({}, defaultOptions, options);
2:57 pm

Nick Reid

Can we delete this entire file? (if so, lets do it)

Can we delete this entire file? (if so, lets do it)

12:24 pm

Mateusz Kwiatkowski

add return annotation

add return annotation

12:24 pm

Mateusz Kwiatkowski

update parameter list

update parameter list