[WIP] tables #2092

Mandily · 2017-07-14T18:19:47Z

I've started some work to clean up the tables in the Solidus admin. I think a lot of the existing table code is redundant now that we've got Bootstrap in the project, so I removed what I could to clean it up, and then added a couple of styles back in to "clean up" what was left. I've just tackled the index pages (except for the stock page) at the moment, and I'd like some feedback before I go through the rest of the views.

Orders

Products

Reports

Promotions

Users

@Sinetheta @jhawthorn @tvdeyen @graygilmore can I get your thoughts on this?

cbrunsdon · 2017-07-17T16:31:07Z

I'm hardly the right person to comment, but everything here looks reasonable to me.

Though we'll likely have to do a semi-large manual QA to make sure there aren't tables somewhere in the deep dark parts of solidus we will need to change.

tvdeyen · 2017-07-17T16:58:37Z

I will make a QA session tomorrow morning

graygilmore · 2017-07-17T17:26:06Z

Cool! Going to run through this now.

graygilmore

spacing is a little odd on the products table (too much space between the image and the name)

It looks like almost all of the tables need some alignment adjustment. I'm going to have a look at fixing these.

graygilmore · 2017-07-17T18:57:53Z

The stock management table in particular falls apart here. I had already been working on some changes to that table. I will coordinate with @Mandily to clean up that area.

tvdeyen · 2017-07-18T07:56:54Z

Nice work @Mandily 👏

Sorry in advance - this is hence the nature of this change - a very long review comment 😞

I really like the new table style. Hence tables are a mayor UI pattern in Solidus admin there is still some work to do.

Walking through the admin I found these issues:

All what @graygilmore said
align-center should be removed from nearly all tables (some examples below) or the header also needs to be centered (like in the "Countries" table -> "States required" column)

Some tables has hard coded inline styles for padding-left or margin-left (like the "Product Properties" and "Reports" table)

Some tables with a "sorting handle" miss an empty header column - otherwise the second column header looks misplaced

We should consider to top align all table cells. Tables that have more than one row though word wrapping look weird (like the "Order / Payments" or the "User / Store Credit" table)

Again, great work. Please feel free to contact me for help. Maybe we should even consider to split this PR up into smaller PRs.

Thanks for the great work!

graygilmore · 2017-07-18T15:51:02Z

@tvdeyen beauty! I nabbed the align-centers yesterday but I'll grab these other ones, too. Just need access to push to @Mandily's PR. I also have a commit that removes the cycle() for odd/even classes.

There might also be some no-border classes that can go away now, too. We'll have clean tables in no time!

Bootstap handles the bulk of the styling and we no longer need these extra styles.

This increases data scanability.

Now that we have new table styles where table headings are aligned to the left by default we want our table cells to follow suit.

These no longer apply any styles. If striping is desired this can be much more easily accomplish by using nth-child in your CSS.

Now that we are using Bootstrap table styles we no longer need to push around pixels like this.

This was necessary before switching to Bootstrap table styles but this class now has no effect.

graygilmore · 2017-07-18T18:37:50Z

I believe I've nabbed all of @tvdeyen's concerns as well as a couple of my own. This PR is rebased and I believe ready for a proper review.

This will make sure that the table headers properly line up with their content below.

Now that the majority of text is left aligned this will make multi-line table cells easier to read.

tvdeyen

Great progress. Thanks @graygilmore for helping out. I found some small glitches here and there. Just commented them below for further reference, but will provide a PR against @Mandily branch.

tvdeyen · 2017-07-18T20:14:11Z

backend/app/assets/stylesheets/spree/backend/shared/_tables.scss

-    border-right: 1px solid $color-border;
-    border-bottom: 1px solid $color-border;
-    vertical-align: middle;
+    vertical-align: top;


This is bootstraps default. We can remove this line

We should consider to add a line-height: 2 to fix the vertical alignment of single line rows. This seems like a large value, but it actually looks pretty good.

Before

After

I actually prefer the before to the after. As a general rule, I try to keep less space between the lines of the same label/list item/paragraph and more between it and whatever is around. It helps keep the item grouped with itself a little better.

I'm with @Mandily on this one, not a fan of the large line-height.

I agree that the large line-height is not ideal, but I also think that the vertical alignment of single line rows should be corrected. What makes me thinking is that the default tables in Bootstraps documentation are all correctly aligned. 🤔

I will have a second look into this problem

@graygilmore @Mandily I know why the vertical alignment for single line rows is off. The action buttons occupy more space then the text and thats why the row is too high.

Solution would be to get rid of the round background.

We should handle this problem in separate PR.
WDYT about this approach?

Separate PR makes sense. I think we're due for another solution in that area entirely.

A bunch of these (this particular example included) can be removed and replaced with the inline editing solution that Martin worked on. I linked a drive document in #2042 for those. I'm ok with this style for the ones that remain.

tvdeyen · 2017-07-19T06:46:21Z

backend/app/views/spree/admin/option_types/_option_value_fields.html.erb

@@ -1,6 +1,6 @@
-<tr class="option_value fields" id="spree_<%= dom_id(f.object) %>"  data-hook="option_value" class="<%= cycle('odd', 'even')%>">
+<tr class="option_value fields" id="spree_<%= dom_id(f.object) %>"  data-hook="option_value">
  <% if f.object.persisted? %>


The <td> only appears if the object is persisted. This looks wrong compared to other sortable tables.

I suggest changing this to:

<td> <% if f.object.persisted? %> <span class="handle"></span> <%= f.hidden_field :id %> <% end %> </td>

and remove the colspan from the name cell.

Before

After

tvdeyen · 2017-07-19T07:05:58Z

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

@@ -65,17 +65,17 @@
      <tr data-hook="admin_products_index_headers">
        <th><%= Spree::Variant.human_attribute_name(:sku) %></th>
        <th colspan="2"><%= sort_link @search,:name, Spree::Product.human_attribute_name(:name), { default_order: "desc" }, {title: 'admin_products_listing_name_title'} %></th>


We should remove the colspan here and add an empty <td> instead.

tvdeyen · 2017-07-19T07:11:07Z

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

-            <td class="align-center"><%= render 'spree/admin/shared/image', image: product.display_image, size: :mini %></td>
+          <tr <%== "style='color: red;'" if product.deleted? %> id="<%= spree_dom_id product %>" data-hook="admin_products_index_rows">
+            <td><%= product.sku %></td>
+            <td><%= render 'spree/admin/shared/image', image: product.display_image, size: :mini %></td>


The image should have a align-center class and we should reduce the column width, so the image looks belonging to the name.

tvdeyen · 2017-07-19T09:18:20Z

@graygilmore @Mandily Mandily#1

The table cell for the sortable handle of the options types form was not present for newly builded records. The cell should always be present only the handle should be hidden.

The stock management table still had a lot of redundant styles.

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

Is there any other work required to get this into an acceptable state?

tvdeyen · 2017-07-31T21:54:08Z

We redundantly declare vertical-align: top on table td although this is Bootstrap's default
Sortable tables should have vertically centered table rows
The reimbursements table of customer return is horizontally centered. It sits in a fieldset with align-center class.

All of the above are solved in Mandily#2

tvdeyen · 2017-07-31T21:54:24Z

@graygilmore FYI

jhawthorn · 2017-08-17T20:22:43Z

Merged as #2159

jhawthorn changed the title ~~Wip tables~~ [WIP] tables Jul 14, 2017

jhawthorn added UI labels Jul 14, 2017

graygilmore suggested changes Jul 17, 2017

View reviewed changes

Mandily and others added 5 commits July 18, 2017 10:46

Transition admin table styles to Bootstrap

c722b9a

Bootstap handles the bulk of the styling and we no longer need these extra styles.

Change header colour to match labels

35d05a1

Align table headers with content below

fce5cc3

This increases data scanability.

Remove center aligned table cells where appropriate

c335e08

Now that we have new table styles where table headings are aligned to the left by default we want our table cells to follow suit.

Remove odd/even table classes

21b73de

These no longer apply any styles. If striping is desired this can be much more easily accomplish by using nth-child in your CSS.

Mandily force-pushed the wip-tables branch from 79fa780 to 21b73de Compare July 18, 2017 17:55

graygilmore added 2 commits July 18, 2017 11:09

Remove inline styling from table cells

ddc84b2

Now that we are using Bootstrap table styles we no longer need to push around pixels like this.

Remove no-border styling from admin tables

9c9d97f

This was necessary before switching to Bootstrap table styles but this class now has no effect.

graygilmore added 2 commits July 18, 2017 13:14

Ensure sorting handle has an empty header column

f932e59

This will make sure that the table headers properly line up with their content below.

Vertically align table cells to the top

ac7918d

Now that the majority of text is left aligned this will make multi-line table cells easier to read.

graygilmore force-pushed the wip-tables branch from ee39f24 to ac7918d Compare July 18, 2017 20:14

tvdeyen reviewed Jul 19, 2017

View reviewed changes

tvdeyen mentioned this pull request Jul 19, 2017

Fixes for wip tables Mandily/solidus#1

Closed

tvdeyen added 3 commits July 19, 2017 10:07

Fixes option values form new record appearance

deece03

The table cell for the sortable handle of the options types form was not present for newly builded records. The cell should always be present only the handle should be hidden.

Fix alignment of image in products table

31ebf30

Optical fixes for stock management table

3b2c894

The stock management table still had a lot of redundant styles.

tvdeyen mentioned this pull request Jul 19, 2017

[RFC] New action icons style #2100

Merged

Merge branch 'master' into wip-tables

cf9542e

graygilmore mentioned this pull request Jul 28, 2017

Pill Component #2123

Merged

1 task

tvdeyen mentioned this pull request Jul 31, 2017

Table vertical alignment fixes Mandily/solidus#2

Closed

tvdeyen mentioned this pull request Aug 9, 2017

Remove the border from images in tables #2143

Merged

jhawthorn added this to the 2.4.0 milestone Aug 9, 2017

jhawthorn mentioned this pull request Aug 16, 2017

New Admin table styles #2159

Merged

jhawthorn closed this Aug 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] tables #2092

[WIP] tables #2092

Mandily commented Jul 14, 2017

cbrunsdon commented Jul 17, 2017

tvdeyen commented Jul 17, 2017

graygilmore commented Jul 17, 2017

graygilmore left a comment

graygilmore commented Jul 17, 2017

tvdeyen commented Jul 18, 2017

graygilmore commented Jul 18, 2017

graygilmore commented Jul 18, 2017

tvdeyen left a comment

tvdeyen Jul 18, 2017

tvdeyen Jul 19, 2017

Mandily Jul 19, 2017

graygilmore Jul 19, 2017

tvdeyen Jul 19, 2017

tvdeyen Jul 19, 2017

tvdeyen Jul 19, 2017

graygilmore Jul 19, 2017

Mandily Jul 19, 2017

tvdeyen Jul 19, 2017

tvdeyen Jul 19, 2017

tvdeyen Jul 19, 2017

tvdeyen commented Jul 19, 2017 •

edited

Loading

graygilmore commented Jul 28, 2017

tvdeyen commented Jul 31, 2017 •

edited

Loading

tvdeyen commented Jul 31, 2017

jhawthorn commented Aug 17, 2017 •

edited

Loading

[WIP] tables #2092

[WIP] tables #2092

Conversation

Mandily commented Jul 14, 2017

cbrunsdon commented Jul 17, 2017

tvdeyen commented Jul 17, 2017

graygilmore commented Jul 17, 2017

graygilmore left a comment

Choose a reason for hiding this comment

graygilmore commented Jul 17, 2017

tvdeyen commented Jul 18, 2017

graygilmore commented Jul 18, 2017

graygilmore commented Jul 18, 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

Before

After

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

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen commented Jul 19, 2017 • edited Loading

graygilmore commented Jul 28, 2017

tvdeyen commented Jul 31, 2017 • edited Loading

tvdeyen commented Jul 31, 2017

jhawthorn commented Aug 17, 2017 • edited Loading

tvdeyen commented Jul 19, 2017 •

edited

Loading

tvdeyen commented Jul 31, 2017 •

edited

Loading

jhawthorn commented Aug 17, 2017 •

edited

Loading