cce-status-update-button

Clone Tools
  • last updated a few seconds ago
Constraints
Constraints: committers
 
Constraints: files
Constraints: dates
Looks good, still missing a little CSS.... which is fine to let slide.... the edge case will become a problem once we make responsive screens (but until then.... eh)

Looks good, still missing a little CSS.... which is fine to let slide.... the edge case will become a problem once we make responsive screens (but until then.... eh)

Add a right margin for the button incase the screen ever gets small enough to overlap I'd also use padding on the aside element to keep the absolute positioned things in the right spots, and I don...

Add a right margin for the button incase the screen ever gets small enough to overlap

I'd also use padding on the aside element to keep the absolute positioned things in the right spots, and I don't think there is a real need for making this a flex box (unless it already had a style in the wrong direction....

Do you really need to pass in the argument to getFunctionalStatusClass ?? if its part of the vm it should be pretty easy to get internally and not in HTML

Do you really need to pass in the argument to getFunctionalStatusClass ?? if its part of the vm it should be pretty easy to get internally and not in HTML

OLMIS-2921: Removed cce-status-update-button module

    • -31
    • +0
    ./cce-status-update-button.module.js
    • -72
    • +0
    ./status-update-button.controller.js
    • -78
    • +0
    ./status-update-button.controller.spec.js
    • -52
    • +0
    ./status-update-button.directive.js
    • -92
    • +0
    ./status-update-button.directive.spec.js
  1. … 6 more files in changeset.
I've created OLMIS-3176 so that we remove this later. I don't want to hold up this ticket for this bug

I've created OLMIS-3176 so that we remove this later. I don't want to hold up this ticket for this bug

<aside class="cce-item-functional-status status-{{vm.inventoryItem.functionalStatus}}"> <p>{{vm.getStatusLabel(vm.inventoryItem.functionalStatus) | message}}</p> <p> <em>{{vm.inventoryItem....
<aside class="cce-item-functional-status status-{{vm.inventoryItem.functionalStatus}}">
  <p>{{vm.getStatusLabel(vm.inventoryItem.functionalStatus) | message}}</p>
  <p>
    <em>{{vm.inventoryItem.lastModifier.firstName}} {{vm.inventoryItem.lastModifier.lastName}}</em>
    {{vm.inventoryItem.modifiedDate | date:'yy/MM/dd HH:mm:ss'}}
  </p>
  <button>Update Status</button>
</aside>
Nick Reid Any ideas on how to improve this markup and keep the style?

Nick Reid Any ideas on how to improve this markup and keep the style?

Nick Reid What do we want about it? It would be awesome to move this ticket forward.

Nick Reid What do we want about it? It would be awesome to move this ticket forward.

Nick Reid What about this unresolved?

Nick Reid What about this unresolved?

Mateusz Kwiatkowski What about this unresolved?

Mateusz Kwiatkowski What about this unresolved?

This is how the ui-sref works internally, it wraps $state.go in a $timeout block. Without it the $state.go will never get called https://review.openlmis.org/static/ql0uca/2static/images/wiki/icons/...

This is how the ui-sref works internally, it wraps $state.go in a $timeout block. Without it the $state.go will never get called

To be honest? I have no idea. The whole button thingy is a tricky thing to reproduce for me.

To be honest? I have no idea. The whole button thingy is a tricky thing to reproduce for me.

I agree, it is still an early version though.

I agree, it is still an early version though.

This entire module seems like an over optimization – I feel like you intend to reuse this in the cce-inventory-list, but the code there is nice and simple This directive just seems to try and do t...

This entire module seems like an over optimization – I feel like you intend to reuse this in the cce-inventory-list, but the code there is nice and simple

This directive just seems to try and do too much, where a little HTML and a button to change the state to the next modal seems like thats all that is needed

This is really div-y. Can we simplify?

This is really div-y. Can we simplify?

Does this get bundled into the application? This should not get bundled into the application – I mean its smaller than our other code, but yuck.... We will chat about this

Does this get bundled into the application?

This should not get bundled into the application – I mean its smaller than our other code, but yuck....

We will chat about this

timeout? why do we need a timeout?

timeout? why do we need a timeout?

Would it make more sense to spyOn getUtilizationStatusLabel to make sure it is called (with the right argument) – rather than search for an element in the DOM? I guess I'm thinking this test would...

Would it make more sense to spyOn getUtilizationStatusLabel to make sure it is called (with the right argument) – rather than search for an element in the DOM?

I guess I'm thinking this test would break if someone changed the layout... where as the intent is really about making sure the inventory details page shows the inventory-item status SOMEWHERE in the view/page/model

I'm marking this as 'needs resolution' because having the element with an ID on it sets off a bunch of warnings in my brain.... (like someone must a a jQuery script attached to it)

This line made me do a double take, as "this" is bound to the function, right?

This line made me do a double take, as "this" is bound to the function, right?

We could, but that would no longer be valid if we ever decide to add more actions buttons here. If we do we'll have to restore this one anyway.

We could, but that would no longer be valid if we ever decide to add more actions buttons here. If we do we'll have to restore this one anyway.

I don't get why we need to add class to button only for test purposes. Could you just change test to find all button type inputs within table?

I don't get why we need to add class to button only for test purposes.
Could you just change test to find all button type inputs within table?

OLMIS-2921: Removed extra line

OK so why there is a inventoryItemId declared in params?

OK so why there is a inventoryItemId declared in params?

remove this line

remove this line

Test purpose.

Test purpose.