Add Tabs Component #870

graygilmore · 2016-02-17T23:59:50Z

First step in #633

I would love some feedback on the JS. I liked bringing this in house. I know there's a library out there that probably does this plus a million things but that's a road I always try to avoid going down.

Re-add the <nav> element: fecc715#commitcomment-16168308
Don't trigger the window's resize: 8c67bc3#commitcomment-16167954
Drop the need for the .tab class: fecc715#commitcomment-16168251
Initial page load on Safari is miscalculating widths and not creating the dropdown.
Shorten the wording on the order submenu: Add Tabs Component #870 (comment)

Before

## After

We're only using this one route, there's no need to generate routes that can't be used.

We want to eventually remove all of the right sidebars from pages but first we need the actual component that we'll be replacing it with.

cbrunsdon · 2016-02-18T00:58:17Z

I think this is a good and important step forward, but I think we should rename "Customer Details" as "Customer" and "Return Authorizations" as RMA in order to get the default list of options on a single line. I think those are just as clear and will hold us over until we can fix the grid width.

graygilmore · 2016-02-18T01:00:22Z

That sounds good to me.

tvdeyen · 2016-02-18T07:05:57Z

Great work @graygilmore 👌🏻.

Although I have to ask this question:

Do we really, as an e-commerce framework, want to maintain a widget library? We discussed this a lot, but I have to raise this question again.

Again. This is awesome work! No doubt, but do we want to hassle with "tabs not working in browser xyz" issues in an e-commerce framework?

graygilmore · 2016-02-18T16:55:33Z

Do we really, as an e-commerce framework, want to maintain a widget library? We discussed this a lot, but I have to raise this question again.

@tvdeyen that's a great question and I'm sure we'll have to draw a line somewhere. What I don't want this project to get in the habit of doing is adding X, Y and Z dependencies that end up causing headaches down the road. It's a valuable discussion to be had, though.

When we run out of room to display the tabs we don't want them to start stacking on each other. Instead, this commit places them cleverly into a dropdown where users can still select them.

We don't want to end up hiding the active tab in the dropdown. This wouldn't give customers a clue as to what page they are on.

Developers shouldn't have to include this empty list in their code.

This is the first step in getting rid of the sidebar in all of the templates.

graygilmore · 2016-02-24T00:01:26Z

@cbrunsdon, @tvdeyen: your feedback has been addressed!

jhawthorn · 2016-02-24T01:21:38Z

👍

tvdeyen · 2016-02-24T06:50:17Z

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

-        <li<%== ' class="active"' if current == 'Return Authorizations' %> data-hook='admin_order_tabs_return_authorizations'>
-          <%= link_to_with_icon 'share', Spree::ReturnAuthorization.model_name.human(count: :other), admin_order_return_authorizations_url(@order) %>
+        <li class="tab <%= "active" if current == "Return Authorizations" %>" data-hook='admin_order_tabs_return_authorizations'>
+          <%= link_to_with_icon 'share', Spree.t(:rma), admin_order_return_authorizations_url(@order) %>


We currently do a bunch of work to use model_name.human everywhere. So introducing a new rma key is not preferred here. So we should change the translation of the mode name here. Although this will have wider consequences.

Using the admin.tab scope would be ok for me.

This commit shortens the wording in the tab headers so that the default max container width will not force tabs to be in the dropdown. We can consider going back to the more verbose wording once the restrictive grid system is changed.

We can safely assume that all `<li>` elements under the `.tabs` namespace should be tabs. Therefor we don't need to use a `.tab` class. This will allow other sidebars to be converted much easier because all we will need to do is add a single class name to the list.

graygilmore · 2016-02-29T22:42:41Z

@tvdeyen once again, updated! 😄

tvdeyen · 2016-03-01T07:21:46Z

I'm a bad person.

Why not leave the RMA translation as it is right now and use model_name.human. I know I gave the hint in the admin.tab direction. Sorry, but we already use it. So, why change it?
We recently decided to switch to Bootstrap. Although this is great work, why don't we use the Bootstrap tabs component here? We could decide to merge this in, add the bootstrap grid, only for shortly after change this to Bootstrap? Although I admire your work, I feel bad about you doing the same work twice.

Murph33 · 2016-03-01T17:06:58Z

My view was that we are just temporarily using RMA instead of Return Authorizations until we change to a wider default content so to change the model_name.human to be RMA would show up in a lot of places instead of just temporarily using admin.tab.rma.

graygilmore · 2016-03-03T21:28:45Z

@tvdeyen the discussion is good! These are all long-term decisions and should be treated as such. The Bootstrap suggestion is good but it doesn't really encompass what we're trying to do here. I have, however, found a lightweight library that may be better than us rolling our own JS. I'm going to play around with it and see if it's worth our time including it instead of the custom implementation here.

graygilmore · 2016-03-04T23:19:31Z

@tvdeyen I explored a library whose entire job was this functionality and it didn't do an important thing: don't collapse "active" elements.

I'm happy with the solution I've provided here and until there is a concrete alternative available I believe we should proceed with this so we can start cleaning up some of those views.

tvdeyen · 2016-03-05T10:49:28Z

@graygilmore in #955 we start to introduce Bootstrap, which provides a Tabs component. Maybe we should have a look if this could work for us and either use your or their component. But having both is kind of silly.

I'm in the don't reinvent the wheel club, as you already know ;)

Again, your work is amazing, but I try to keep the maintenance burden we already have as low as possible.

So, if you can invest some time to try the Bootstrap component, this would be very much appreciated.

Murph33 · 2016-03-05T16:05:28Z

@tvdeyen, It came up that @graygilmore did check out the bootstrap tabs and it doesn't encompass what we're looking for during the meeting #870 (comment). Also the current plan is to add the bootstrap .js components on a need to have basis so the tabs.js wouldn't be in unless someone explicitly adds it. As a beginner I can attest to the ease of using @graygilmore's tabs as well :)

jhawthorn · 2016-03-08T00:41:07Z

I gave this a look today. To clarify a few points:

Bootstrap's tab.js doesn't help us here at all. tab.js adds the ability to hide and show a div for each tab on the same page, which we aren't doing. Our tabs are links to other pages. It doesn't provide the functionality this does of collapsing items into a dropdown if they are too large. (bootstrap does also have collapse.js, but that's slightly different, and not quite what we're looking for here)

We could use bootstrap's css here, but that will either require changing markup (which this PR carefully avoided), or require additional complexity to use the same mixins in out _tabs.scss. The latter I think is worth investigating (I think bootstrap's tabs have a better look than these), but is outside the scope of this PR, especially as we haven't merged bootstrap yet. I'd be happy to look into styling after both this and bootstrap are merged

tvdeyen · 2016-03-08T06:40:43Z

Thanks John.

🚀 🚢

Add Tabs Component

jhawthorn · 2016-03-08T23:25:15Z

Thank you everyone

mtomov · 2016-03-10T12:25:22Z

Thank you, looks great!

graygilmore added 2 commits February 17, 2016 14:58

Restrict style guide routes to index only

92790cc

We're only using this one route, there's no need to generate routes that can't be used.

Add basic tabs component to the style guide

76206d5

We want to eventually remove all of the right sidebars from pages but first we need the actual component that we'll be replacing it with.

graygilmore added 3 commits February 23, 2016 10:25

Condense tabs into dropdown when there is no space

b803e08

When we run out of room to display the tabs we don't want them to start stacking on each other. Instead, this commit places them cleverly into a dropdown where users can still select them.

Make sure the active tab is always visible

1c9de81

We don't want to end up hiding the active tab in the dropdown. This wouldn't give customers a clue as to what page they are on.

Dynamically create the dropdown for tabs

75c51cd

Developers shouldn't have to include this empty list in their code.

graygilmore force-pushed the replace_sidebar_with_tabs branch 2 times, most recently from 76d9709 to 2705a6e Compare February 23, 2016 23:32

Use tabs instead of a sidebar on the Order pages

1bead53

This is the first step in getting rid of the sidebar in all of the templates.

graygilmore force-pushed the replace_sidebar_with_tabs branch from 0b52b74 to 3385cde Compare February 23, 2016 23:58

tvdeyen reviewed Feb 24, 2016
View reviewed changes

Shorten wording in order submenu tab headers

6c57f20

This commit shortens the wording in the tab headers so that the default max container width will not force tabs to be in the dropdown. We can consider going back to the more verbose wording once the restrictive grid system is changed.

graygilmore force-pushed the replace_sidebar_with_tabs branch from 19d4aa8 to 8bb3090 Compare February 26, 2016 22:07

graygilmore force-pushed the replace_sidebar_with_tabs branch from 81ed9e6 to 6706568 Compare February 29, 2016 22:34

Murph33 mentioned this pull request Mar 3, 2016

Replace settings configuration menu with separate tabs partials #952

Closed

jhawthorn added a commit that referenced this pull request Mar 8, 2016

Merge pull request #870 from graygilmore/replace_sidebar_with_tabs

8c939b8

Add Tabs Component

jhawthorn merged commit 8c939b8 into solidusio:master Mar 8, 2016

Murph33 mentioned this pull request Mar 8, 2016

Replace settings configuration menu with separate tabs partials #975

Merged

Murph33 mentioned this pull request May 17, 2016

Use tabs in user edit views #1157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Tabs Component #870

Add Tabs Component #870

graygilmore commented Feb 17, 2016

cbrunsdon commented Feb 18, 2016

graygilmore commented Feb 18, 2016

tvdeyen commented Feb 18, 2016

graygilmore commented Feb 18, 2016

graygilmore commented Feb 24, 2016

jhawthorn commented Feb 24, 2016

tvdeyen Feb 24, 2016

graygilmore commented Feb 29, 2016

tvdeyen commented Mar 1, 2016

Murph33 commented Mar 1, 2016

graygilmore commented Mar 3, 2016

graygilmore commented Mar 4, 2016

tvdeyen commented Mar 5, 2016

Murph33 commented Mar 5, 2016

jhawthorn commented Mar 8, 2016

tvdeyen commented Mar 8, 2016

jhawthorn commented Mar 8, 2016

mtomov commented Mar 10, 2016

Add Tabs Component #870

Add Tabs Component #870

Conversation

graygilmore commented Feb 17, 2016

Before

cbrunsdon commented Feb 18, 2016

graygilmore commented Feb 18, 2016

tvdeyen commented Feb 18, 2016

graygilmore commented Feb 18, 2016

graygilmore commented Feb 24, 2016

jhawthorn commented Feb 24, 2016

tvdeyen Feb 24, 2016

Choose a reason for hiding this comment

graygilmore commented Feb 29, 2016

tvdeyen commented Mar 1, 2016

Murph33 commented Mar 1, 2016

graygilmore commented Mar 3, 2016

graygilmore commented Mar 4, 2016

tvdeyen commented Mar 5, 2016

Murph33 commented Mar 5, 2016

jhawthorn commented Mar 8, 2016

tvdeyen commented Mar 8, 2016

jhawthorn commented Mar 8, 2016

mtomov commented Mar 10, 2016