Pill Component #2123

graygilmore · 2017-07-28T18:41:36Z

This PR introduces a new "pill" component. This component is used to display the state of something through six distinct colors:

Complete
Active
Inactive
Pending
Warning
Error

This should make scanning tables significantly easier and provide a slightly more modern feel to the admin (looks great with the new tables in #2092).

TODO

fix test errors

Style Guide

Orders (before)

Orders (after)

Promotions (before)

Promotions (after)

Mandily · 2017-07-28T19:07:26Z

I like the content of what you're trying to say in the writeup in the style guide, but could we write it in a clearer way? Maybe something like this:

"Pill styling adds a second, easily scannable way to recognize status. They're most commonly used in, but not restricted to tables."

tvdeyen · 2017-07-28T19:25:22Z

I like that! 😍

Maybe it's just me, but shouldn't complete be green? I assume shop owners love to see lots of green in their orders table.

tvdeyen

Again, a great improvement. Really appreciate the work went into it 👏

I left some comments about how we should treat presentation of values and re-organizing the git history.

tvdeyen · 2017-07-28T19:30:01Z

backend/app/assets/stylesheets/spree/backend/components/_pills.scss

+  display: inline-block;
+  padding: 2px 10px;
+  border-radius: 10px;
+  font-size: 11px;


The font-size should have a relative unit.
Also the paddings.

Any particular reason?

If you raise the body font size you want to grow this as well. The same with the passings. They should be relatively to the font size to keep the ratios.

paddings not passings. (Sorry iOS)

I'm more curious about who the perspective is here. This is a very specific component that I have sized specifically. If users viewing the admin bump up their font size this will scale along with it.

If we decide to change the root font size of the admin we'll need to go through components one by one and figure out what needs to change. I'd normally be happy to use rem but our admin root font size is 13px which I can see changing in the future but I wouldn't want this component to change.

I like us to move forward to relative units all over the place. That’s the way all frontend frameworks went for a reason.

I see the problem with the small base font size we have. So, I won’t insist to make this change right now.

But, could you please make this font size a variable then?

tvdeyen · 2017-07-28T19:36:10Z

backend/app/views/spree/admin/promotions/index.html.erb

+          </td>
+          <td class="align-center">
+            <%= promotion.expires_at.to_date.to_s(:short_date) if promotion.expires_at %>
+          </td>


You are changing the table far more than just adding the pill component. Could you please do this in a separate commit, or not even PR?

tvdeyen · 2017-07-28T19:37:46Z

core/config/locales/en.yml

-        usage_limit: Overall Usage Limit
+        starts_at: Start
+        status: Status
+        usage_limit: Usage Limit


These changes do not belong in this PR IMO. At least it should be a separate commit.

tvdeyen · 2017-07-28T19:39:45Z

backend/app/helpers/spree/admin/reimbursements_helper.rb

-      end
-    end
-  end
-end


Could we move this removal in a separate commit after this commit?

tvdeyen · 2017-07-28T19:40:57Z

backend/app/helpers/spree/admin/stock_transfers_helper.rb

@@ -17,10 +17,6 @@ def stock_transfer_edit_or_ship_path(stock_transfer)
        end
      end

-      def stock_transfer_status(stock_transfer)
-        stock_transfer.closed? ? Spree.t(:closed) : Spree.t(:open)
-      end


this as well, please

tvdeyen · 2017-07-28T19:54:18Z

backend/app/views/spree/admin/customer_returns/_return_item_decision.html.erb

@@ -39,7 +39,9 @@
          <%= return_item.acceptance_status_errors %>
        </td>
        <td class-"align-center">
-          <%= return_item.reception_status.humanize %>
+          <span class="pill pill-<%= return_item.reception_status %>">
+            <%= return_item.reception_status.humanize %>


We should only use humanize if we pass it through I18n before and humanize the fallback translation.

Spree.t(return_item.reception_status, scope: 'return_item_reception_statuses', default: return_item.reception_status.humanize)

tvdeyen · 2017-07-28T19:55:44Z

backend/app/views/spree/admin/customer_returns/_return_item_selection.html.erb

@@ -38,7 +38,9 @@
          <%= return_item.display_amount %>
        </td>
        <td class="align-center">
-          <%= return_item.inventory_unit.state %>
+          <span class="pill pill-<%= return_item.inventory_unit.state %>">
+            <%= return_item.inventory_unit.state.titleize %>


Same comment as above. I suggest to make a separate commit (not even a PR) first that changes the presentation via I18n.

tvdeyen · 2017-07-28T19:56:12Z

backend/app/views/spree/admin/orders/confirm/_payments.html.erb

+                payment.state,
+                scope: :payment_states,
+                default: payment.state
+              ).titleize %>


tvdeyen · 2017-07-28T19:57:42Z

backend/app/views/spree/admin/orders/index.html.erb

-            <td class="align-center"><span class="state <%= order.shipment_state %>"><%= Spree.t("shipment_states.#{order.shipment_state}") if order.shipment_state %></span></td>
+        <td class="align-center">
+          <span class="pill pill-<%= order.state.downcase %>">
+            <%= Spree.t("order_state.#{order.state.downcase}").titleize %>


Same comment because of titleization applies here

tvdeyen · 2017-07-28T19:59:39Z

backend/app/views/spree/admin/users/items.html.erb

-                <% if Spree::Order.checkout_step_names.include?(:delivery) && order.shipment_state %>
-                  <div class="state <%= order.shipment_state %>"><%= Spree.t("shipment_states.#{order.shipment_state}") %></div>
-                <% end %>
-              </td>


Should be a separate commit as well

tvdeyen · 2017-07-28T20:06:40Z

The build errors are actually related to the reimbursements helper removal.

Mandily · 2017-07-28T20:11:04Z

re: "Shouldn't complete be green?"

Green is usually associated with an action - like "go" or "active" in this case. In the case of grey being used for complete, it means that a thing is done and you don't have to worry about doing anything else for it. The colours stand out more than the greys because they're the things still in flux that an admin might want to keep an eye on. The neutral things are the ones they don't have to worry about.

The caveat here being that most if not all of my colour theory is based on North American norms. Maybe these colours mean different things to you.

Make sense?

graygilmore · 2017-07-28T20:11:27Z

Maybe it's just me, but shouldn't complete be green? I assume shop owners love to see lots of green in their orders table.

No. Green in this case specifies something that is active. It gives the feeling of movement and that something is currently happening. A complete state is something that needs no further action and is finished, and the grey-blue color better helps signal that.

Edit: ha! Same time as @Mandily.

tvdeyen · 2017-07-28T21:32:53Z

Re: Re: Green

This makes total sense. Thanks for the explanation.

This will allow us to better display the status of many of the resources in a store.

This will give admins a much better idea of the state of each of the promotions by: 1. Having an "active" or "inactive" column (to use the pill component in a later commit. 2. Displaying both the start and end date 3. Reducing the complexity around some of the table header wording

Orders, payments, shipments and other resources have their own states that need to hook into our new pill component. Instead of modifying each of these values to line up to our pills' values we can just extend each of the states through Sass. Normally I like to avoid using `@extend` but this is the perfect use case for us.

houndci-bot · 2017-07-31T18:29:49Z

backend/spec/features/admin/users_spec.rb

-        end
-      end
+    context "number" do
+      let(:text_match_1) { "R123"}


Space missing inside }.

houndci-bot · 2017-07-31T18:29:50Z

backend/spec/features/admin/users_spec.rb

-        end
-      end
+    context "number" do
+      let(:text_match_1) { "R123"}


Space missing inside }.

These are a lot easier to read in the admin if they use the correct sentence-case structure. This is preferable than modifying the values directly in the markup with something like `titleize` because that may not work for different languages.

The state itself is currently being displayed in the admin. Adding a translation will better help multilingual stores.

This helper file only contained one method that is no longer in use after switching the state display of a reimbursement to use the pill component.

This is no longer in use after switching this display to use the pill component.

Now that we are using pills in the table it should be much easier to get an idea of the state of an order.

The other states being displayed here can be downright false. If an item has been shipped to the customer and then returned back to the store you would have no way of knowing from this view.

graygilmore · 2017-07-31T18:44:56Z

Wooo tests are green. @tvdeyen I think I've addressed all of your other concerns. I went a little overly verbose with the number of commits but that should make things a little cleaner to understand in the future.

tvdeyen

These commit messages are perfect. They are really helpful. Thanks for addressing

Great feature 👍

jhawthorn · 2017-08-01T23:26:24Z

backend/app/views/spree/admin/shared/_order_summary.html.erb

+        <%= Spree.t(
+          @order.state,
+          scope: :order_state,
+          default: @order.state.humanize


I'd rather not use humanize as a default. We want to see where we are missing translations (and avoid the cost of calling the inflector)

We want to see where we are missing translations (and avoid the cost of calling the inflector).

graygilmore · 2017-08-02T01:24:42Z

core/config/locales/en.yml

@@ -1645,6 +1645,7 @@ en:
      pending: Pending
      processing: Processing
      void: Void
+      invalid: Invalid


Nice! Thanks for that.

Eth3rnit3 · 2017-10-18T11:47:55Z

I have this error message after the update to 2.4 do you have any idea where it might come from?
server-side :
ActionView::Template::Error (".state.sold" failed to @extend ".pill-sold". The selector ".pill-sold" was not found. Use "@extend .pill-sold !optional" if the extend should be able to fail. ): 4: <meta content="width=device-width, initial-scale=1.0, maximum-scale=1" name="viewport"> 5: <%== meta_data_tags %> 6: <%= canonical_tag(current_store.url) %> 7: <%= favicon_link_tag 'favicon.ico' %> 8: <%= stylesheet_link_tag 'spree/frontend/all', media: 'screen' %> 9: <%= csrf_meta_tags %> 10: <%= javascript_include_tag 'spree/frontend/all' %>

mamhoff · 2017-10-18T12:16:05Z

@Eth3rnit3 Have you overridden any style sheets in your host app? Compare them with the ones in Solidus 2.4. You're probably not requiring the pills SCSS file.

Eth3rnit3 · 2017-10-18T12:18:51Z

yes only the file variables_override. scss I'm going to compare it right now and I'm telling you

Eth3rnit3 · 2017-10-18T12:22:34Z

no unfortunately the file is empty in solidus

Eth3rnit3 · 2017-10-18T12:35:59Z

you're right, if I add the line:

@import 'spree/backend/components/pills'

to my file, it works ; -) Thank you

Eth3rnit3 · 2017-10-18T12:50:36Z

not really sorry, actually.
only a css problem that made a mistake earlier

Eth3rnit3 · 2017-10-18T13:29:41Z

actually the problem was that I overloaded the $state class in my variables_override file. thank you, maybe we'll see you soon.

graygilmore force-pushed the pills branch from daf9a3f to 8de5af0 Compare July 28, 2017 18:54

tvdeyen requested changes Jul 28, 2017

View reviewed changes

graygilmore force-pushed the pills branch from 8de5af0 to e6963e3 Compare July 28, 2017 22:52

graygilmore added 6 commits July 31, 2017 09:49

Add vertical spacing between style guide code samples

a18aec5

Add a pill component to the admin

c6a165e

This will allow us to better display the status of many of the resources in a store.

Add the pill component to the promotions index

85b2168

Add the pill component to the adjustment reasons index

8668ce2

graygilmore force-pushed the pills branch from e6963e3 to b278b64 Compare July 31, 2017 18:29

houndci-bot reviewed Jul 31, 2017

View reviewed changes

backend/spec/features/admin/users_spec.rb Outdated

end

end

context "number" do

let(:text_match_1) { "R123"}

Copy link

houndci-bot Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space missing inside }.

houndci-bot reviewed Jul 31, 2017

View reviewed changes

backend/spec/features/admin/users_spec.rb Outdated

end

end

context "number" do

let(:text_match_1) { "R123"}

Copy link

houndci-bot Jul 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space missing inside }.

graygilmore added 12 commits July 31, 2017 11:31

Add the pill component to the cancelations index

e6266bc

Use the pill component on the order risk analysis

2414136

Use the pill component on payments confirmation

7429e4a

Use the pill component on the payment methods index

e01d8b8

Use the pill component for states of payments

b57d253

Add reimbursement states to the English translations

5ec5c3b

The state itself is currently being displayed in the admin. Adding a translation will better help multilingual stores.

Use the pill component for states of reimbursement

72bd2da

Remove ReimbursementsHelper

ff42abd

This helper file only contained one method that is no longer in use after switching the state display of a reimbursement to use the pill component.

Add translations for ReturnItem state machine

7240930

Use the pill component for states of return items

ee4485b

Use translations for inventory unit states

d2f7afc

graygilmore added 11 commits July 31, 2017 11:31

Use the pill component for inventory unit states

d27be66

Use translations for return authorization states

f8a029c

Use the pill component for states of return authorizations

444cb46

Use the pill component for named type listings

db94b8d

Use the pill component for state of stock locations

76dc2dd

Use the pill component on the state of stock transfers

639091a

Remove the stock_transfer_status helper method

2548f8c

This is no longer in use after switching this display to use the pill component.

Use the pill component for displaying order information

add4123

Remove state border from orders table

f577421

Now that we are using pills in the table it should be much easier to get an idea of the state of an order.

Only display the order's state in a user's Items view

9e74e65

The other states being displayed here can be downright false. If an item has been shipped to the customer and then returned back to the store you would have no way of knowing from this view.

Use the pill component for order states on user's items

3291601

graygilmore force-pushed the pills branch from b278b64 to 3291601 Compare July 31, 2017 18:31

tvdeyen approved these changes Jul 31, 2017

View reviewed changes

tvdeyen added changelog:solidus_backend Changes to the solidus_backend gem UI labels Aug 1, 2017

jhawthorn reviewed Aug 1, 2017

View reviewed changes

Remove default from state machine translations

1e5f228

We want to see where we are missing translations (and avoid the cost of calling the inflector).

jhawthorn approved these changes Aug 1, 2017

View reviewed changes

Add missing payment_states.invalid translation

43eef78

graygilmore commented Aug 2, 2017

View reviewed changes

jhawthorn merged commit ec13f80 into solidusio:master Aug 2, 2017

graygilmore deleted the pills branch October 19, 2017 16:50

Pill Component #2123

Pill Component #2123

Conversation

graygilmore commented Jul 28, 2017 • edited Loading

TODO

Style Guide

Orders (before)

Orders (after)

Promotions (before)

Promotions (after)

Mandily commented Jul 28, 2017

tvdeyen commented Jul 28, 2017

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen commented Jul 28, 2017

Mandily commented Jul 28, 2017

graygilmore commented Jul 28, 2017 • edited Loading

tvdeyen commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graygilmore commented Jul 31, 2017

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eth3rnit3 commented Oct 18, 2017

mamhoff commented Oct 18, 2017

Eth3rnit3 commented Oct 18, 2017

Eth3rnit3 commented Oct 18, 2017

Eth3rnit3 commented Oct 18, 2017 • edited Loading

Eth3rnit3 commented Oct 18, 2017

Eth3rnit3 commented Oct 18, 2017

graygilmore commented Jul 28, 2017 •

edited

Loading

graygilmore commented Jul 28, 2017 •

edited

Loading

Eth3rnit3 commented Oct 18, 2017 •

edited

Loading