-
-
Notifications
You must be signed in to change notification settings - Fork 531
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
Custom units docs #5082
Custom units docs #5082
Conversation
@@ -19,7 +19,7 @@ | |||
<td class="p-4 d-flex flex-wrap"> | |||
<% partner_request.item_requests.map do |item| %> | |||
<span class="p-1 mr-1 mb-2 lg:mb-0 border border-dark rounded-1"> | |||
<%= item.quantity %> <%= item.name %> | |||
<%= item.quantity %> <%= item.name_with_unit %> |
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.
Bug fix 1 - the partner request view didn't include the units
@@ -22,7 +22,7 @@ | |||
<td><%= line_item.name %></td> | |||
<td><%= line_item.quantity %></td> | |||
<% if Flipper.enabled?(:enable_packs) && @partner_request.item_requests.any?( &:request_unit ) %> | |||
<td><%= line_item.request_unit %></td> | |||
<td><%= line_item.request_unit&.capitalize&.pluralize(line_item.quantity.to_i) %></td> |
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.
Bug fix 2 - the partner request review pop-up didn't pluralize units
@cielf I added in the preamble you suggsted on slack into a new section at the top. Here is the evergreen branch preview link -- https://github.com/rubyforgood/human-essentials/blob/custom-units-docs/docs/user_guide/bank/special_custom_units.md |
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.
@awwaiid I've made some proofing/rewording changes -- a little bit of making the things capitalized that should be, a little providing variety in words, a little this that and the other thing.
I think we might want to expand on the last line -- we don't need to show the whole thing, but maybe something like 'e.g., If you allow the unit 'boxes' on Pads, you will see a column for "Pads" and one for "Pads - boxes."
Read it over and see if I've introduced any new errors, but otherwise, I think we let Myranda tell us if it's confusing?
Thanks @cielf! I looked through all of the changes you made and they are 👍 |
I think it's good enough to have the next step in it's refinement be beta testing with our designated user. |
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.
In other words, LGTM.
Just added "in beta testing" so that people don't see this and become confused that they can't have it yet.
@awwaiid: Your PR |
Needs some editing feedback!