-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Admin order payments ui cleanup #2411
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left a comment on currency input type, I'm 👍 on all other changes, thanks!
<div class="input-group-addon number-with-currency-symbol"> | ||
<%= ::Money::Currency.find(@order.currency).symbol %> | ||
</div> | ||
<%= number_field_tag :amount, payment.amount, class: 'js-edit-amount align-right form-control', step: :any %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to use "text" input for currency fields, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes. This is a left over from my investigation. Thanks! Will fix that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kennyadsl Fixed it
625220e
to
819bdad
Compare
@@ -0,0 +1,24 @@ | |||
Spree.Views.Payment.PaymentRow = Backbone.View.extend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the file should be in javascript instead coffee.
From backend README:
Though we have existing CoffeeScript files, any new files should be in javascript (ES5).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you are right, good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do, thanks!
|
||
onEdit: (e) -> | ||
e.preventDefault() | ||
@$el.addClass("editing") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely is outside the scope of this changes.
But we have similar backbone views that use a conditional value editing
with a handlebars template. (ex: orders/shipping_methods) I think that approach is better than show/hide with css.
Maybe we can create a common view for use in this cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely. Will see if I can sneak this into this PR
After going full house bootstrap the form looks a little off. This re-structures the form so it fits better into the admin interface.
All our backbone views live in the spree/backend/views folder. Also uses our Spree javascript object to store the instance instead of a global.
Instead of defining the model everytime we render the payment screen we define a model in the spree/backend/models folder and initialize that when we render the payments screen.
The amount needs a min-width in order to not be too small in narrow areas. One might not be able to see the value. The addon shoud not have a fixed width, but a min-width so it grows when necessary, but not take too much space it actually does not need.
We sometimes want to vertically align table rows to the middle, not to the top as we do by default now. This new class allows to do so.
It is ok to just call this state "State" instead of "Payment State". We use this attribute in lots of tables and the space is limited.
The order payments table has lots of information and lacks some structure. By moving the amount to the right of the table and give it some as well as align it to the right this important piece of information is now better visible. If a user wants to change the amount of an uncaptured payment the form is now right by the action icons and not somewhere in the middle. By further shorten the creation date format we gain some valuable extra space.
Per our definition newly introduced files should be JS instead of Coffeescript.
819bdad
to
3d135fe
Compare
@kennyadsl made the requested changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks!
Extracted from #2255
Made some changes to the admin order payments UI:
Screenshots