-
-
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
Improved promotions form #1509
Improved promotions form #1509
Conversation
Thanks for working on this John. I feel it is an improvement. |
Very 👍 on this! If we merge #1524 we'll want to take that into account also yeah? |
This is awesome! 👍 Way better than what we have now. With regards to the questions in your "work to do" section:
|
Definite huge improvement. Whether part of one PR or not, I think the "view" and "edit" screens for existing Promotion need attention too. Might make sense to make part of same PR for 'edit' at least, since that's actually the same partial I think, if you don't attend to it it could make it even worse. One of the problems with "edit" screen is it is currently, is that it's not possible to show actual base code for multi-code promotions, since it's not preserved. Current edit screen just picks the first code in the database, and labels it 'base code' in the UI, which isn't right. PR #1410 would make promotions remember their base code, which could allow view/edit on an existing promotion to make more sense. |
Though we do allow combinations in the data-model (both a code and a path, for example), I think it's an uncommon use case that we might as well just hide in the UI for simplicity.
Sounds good. |
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've left a completely minor nit, but am fine with the UI changes and like the new code better (like splitting up the Coffeescript by calculator).
</div> | ||
</div> | ||
|
||
<div id="expiry_fields" class="three columns omega"> | ||
<%= f.field_container :overall_usage_limit do %> |
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.
This is a super-nit. Should we be mixing old- and new-style Hash syntax in the same line? I feel like if we touch the line anyways, we could just switch it over to new if nothing stands against it.
48d85ce
to
708d915
Compare
708d915
to
d3d4a53
Compare
This should be slightly nicer than seeing an error after a round trip to the server. Disabled elements are skipped for HTML validations, so this works with the hidden elements.
d3d4a53
to
6b9b720
Compare
I have rewritten this to work after the changes in #1524. I've gone with a very simple solution for the "edit" action, and I'd like to make further improvements in a separate PR. We didn't previously allow editing codes on existing promotions. The code field was marked readonly and the controller didn't support it. I've made this (hopefully) more clear by just printing the code as text. This is ready for review |
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.
Great improvement. I created a follow up for further improvements #1702
This is a big usability improvement from a small change. 👍 Could we provide some additional indication to users that clicking a radio button will reveal further controls? Most radio buttons don't do that, and the initial state has no controls to the right, so it might not be obvious. This could be as simple as adding an ellipsis ... after the labels for those radio buttons that bring up other fields. Beyond the scope of this pull request, I feel like these four radio buttons are conceptually closer to being Promotion Rules than attributes of the overall Promotion. They're all conditions under which the promotion will be activated. I appreciate that would entail a significant change to the code, but I wonder whether promotions would be easier to understand if we could push more features down into Rules. |
@@ -0,0 +1,66 @@ | |||
<% activation_type = params[:activation_type] || 'single_code' %> | |||
<div class="row" id="js_promotion_activation"> |
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.
@jhawthorn why you are not using dash but underscores to separate words for all elements ids?
I think it is universally agreed that the promotions form is too complicated and confusing.
Existing form:
I felt that one of the biggest problems with this form is that it is difficult for an inexperienced user (and even users with years of spree/solidus experience) to know how to create a new promotion that activates when they want it.
A user needs to know
To make this easier to use, I've created a set of radio buttons to help guide a user to their desired behaviour without knowing the meanings behind all these magic fields.
These radio buttons are only used by JS and aren't persisted.
Redesigned page
Designing things is usually not my forte, but I think this is an improvement. I started by splitting the fields into three sections, with the last one being the new radio buttons.
Radio buttons
Work to do: