Skip to content
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

Admin navigation sidebar #509

Merged
merged 18 commits into from Dec 17, 2015
Merged

Admin navigation sidebar #509

merged 18 commits into from Dec 17, 2015

Conversation

Sinetheta
Copy link
Contributor

  • Adds a full height sidebar to the admin area
  • Moves the admin nav from the header to the sidebar
  • Nests the subnav in the main nav (visible as an open "accordion")
  • Adds a flyout for not active subnavs

screen shot 2015-11-13 at 1 54 09 pm

This will be accompanied by a tiny patch to solidus_auth_devise since the current user nav items (logged in as, account, logout) have been moved as well.

resolves #498
resolves #505
advances #487

@jhawthorn jhawthorn changed the title Admin nav Admin navigation sidebar Nov 13, 2015
</div>
<nav class="admin-menu">
<ul data-hook="admin_tabs">
<%= render :partial => 'spree/admin/shared/tabs' %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're touching all of these files anyways I'd love for us to switch to the new hash syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No prob

@tvdeyen
Copy link
Member

tvdeyen commented Nov 13, 2015

NICEEE!!! 🎉

display: table;
table-layout: fixed;
a {
display: inline-block;
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's inline-block and width: 100% we should just be able to one-line this with display: block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@Sinetheta
Copy link
Contributor Author

It doesn't address the generation of the nav at all @tvdeyen, just the layout. I'm totally open to creating this markup from whatever system we deem worthy. Right now I've just shuffled the existing partials around.

&:before {
position: absolute;
left: 1em;
transform: translateX(-50%);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we care about Safari 8 (previous version) we'll need a mixin here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always forget the prefixer.

@Sinetheta Sinetheta force-pushed the admin-nav branch 2 times, most recently from 5dac418 to f3c9eb7 Compare November 13, 2015 22:05
@@ -0,0 +1,5 @@
<div class='sidebar'>
Copy link
Contributor

Choose a reason for hiding this comment

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

Single quoted attributes 😢

But more importantly: the partial is called _sidebar_left but the class name is sidebar and the children use .sidebar-left-header.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's only one main sidebar I'd prefer these to all just be sidebar-#{thing}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just hedging my bets...

@Sinetheta Sinetheta force-pushed the admin-nav branch 2 times, most recently from 1531885 to ea207d5 Compare November 13, 2015 22:21
@@ -0,0 +1,5 @@
<div class="sidebar">
<%= render partial: 'spree/admin/shared/sidebar_left_header' %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be sidebar_left_#{thing}? Seems like this is the one and only main sidebar. Until we're planning on having a right sidebar I'd like to kill the left.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's currently a thing called "sidebar" which refers to the right content of a page with a 3rd level nav (e.g., /admin/general_settings/edit).

@tvdeyen
Copy link
Member

tvdeyen commented Nov 13, 2015

@Sinetheta it's already an improvement ;)

BTW: In the settings menu, the right sub nav is somewhat misplaced:

solidus administration general settings 2015-11-13 23-24-44

@tvdeyen
Copy link
Member

tvdeyen commented Nov 13, 2015

I think the settings sub navigation should also be inside a flyout. The right subnavigation normally is based on one record (ie. Product 1 -> Images for Product 1). The settings on the contrary are not related to one particular resource and it would be very nice to be able to quickly reach the sub items.

@tvdeyen
Copy link
Member

tvdeyen commented Nov 13, 2015

The user login menu is not showing for me (fresh sandbox install):

solidus administration orders 2015-11-13 23-33-50

<div class="sidebar">
<%= render partial: 'spree/admin/shared/sidebar_left_header' %>
<%= render partial: 'spree/admin/shared/menu' %>
<%= render partial: 'spree/admin/shared/sidebar_left_footer' %>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe now is a good time to move all sidebar related stuff into it's own folder spree/admin/sidebar?

spree/admin/sidebar
  |-- _header.html.erb
  |-- _menu.html.erb
  |-- _footer.html.erb

@tvdeyen
Copy link
Member

tvdeyen commented Nov 13, 2015

Adding position: relative to body would make the sidebar 100% height:

solidus administration products 2015-11-13 23-47-34

Also, adding a background color (i.e. #EFF5FC) would be great to separate the sidebar from main content.

@Sinetheta
Copy link
Contributor Author

Thanks for pulling this down to test @tvdeyen!

  • I'll check the settings menu when I'm back in office.
  • Having the settings menu in a flyout (are we talking about the "sidebar" 3rd level of nav in general?) might not be possible, as it seems quite large depending on the page.
  • The user login menu will require an update to solidus auth devise, since it's defaced in and I've renamed the partial.
  • I think a folder for the sidebar partials is a great idea
  • I'll check the position relative
  • RE: sidebar color. What do you think about coloring it in @Mandily to provide some contrast there? I think all of the colours are up for debate.

@Sinetheta Sinetheta force-pushed the admin-nav branch 4 times, most recently from c11911d to 808f5cf Compare November 16, 2015 19:58
@Sinetheta
Copy link
Contributor Author

So, it looks like the remaining spec are failing due to overlap of the flash with what is now a higher content area.

screen shot 2015-11-16 at 1 39 01 pm

@Mandily would like to scope creep those into dismisable alerts at the top of the main content area.
I'd like to move them to the bottom.

screen shot 2015-11-16 at 3 07 52 pm

#logo { height: 40px }
img {
vertical-align: middle;
margin-top: -12px; // hand-tweaking because the solid logo is bottom heavy
Copy link
Member

Choose a reason for hiding this comment

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

You should tweak the logo instead. Add some padding in the image, for instance. Lots of shops change the admin logo, so all would have to deal with this. Not ideal IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. That corner is bound to be problematic. Either it's a part of the sidebar which "happens" to be the same height as the .page-title or part of the main content with happens to be the same width as the sidebar.

I'm thinking that the latter might help with this positioning problem since the sidebar currently has a static width.

@tvdeyen
Copy link
Member

tvdeyen commented Nov 16, 2015

@jhawthorn I agree. Good point.

@Sinetheta
Copy link
Contributor Author

@tvdeyen in regards to the third level of navigation, I'm going to leave it where it is right now (on the right side of the main content). But I believe that @Mandily is considering a "tabbed layout" for the main content area, where the 3rd level of nav would be the tabs.

The logo is aligned with the now highest level page item,
the .page-title

The current user links which are defaced in by solidus_auth_devise
are now placed in a sticky footer for the sidebar.
This is the little triangle guy which makes popups and dropdowns
"belong" to something.

This is the simplest form of those dongles, an :after pseudo element.

In order to add a border to the caret we could overlay it on a
:before element and use some css magic to offset them slightly.

This would be a great thing to demo on a style guide/
The notable features here are

- Vertical list of main nav items with icons
- Active subnav is shown as an expanded accordion
- Other subnavs are exposed as flyouts on hover
To distinguish the left sidebar (main navigation) from the right
sidebar (3rd level of navigation which appears in the content area)
we will refer to it as the .admin-nav

This also fixes a css spillover from using the class .sidebar which
was causing the right sidebar to be misplaced.

We will, however, avoid renaming templates when at all possible
to prevent breaking users Deface overrides.
Instead of using deface we prefer an empty template to override.
To prevent them from laying on top of the, now higher, main content.
Safer than specific vertical offsets is a vertically centered image.
Although this menu is inserted by the gem solidus_auth_devise,
the styles have much more to do with now this particular area
is styled.

Since we're also talking about a very small amount of css, in a
nearly ubiquitous gem, I feel comfortable including these styles
in solidus itself.
This prevents the login nav from appearing to "float" beneath the
admin nav when there's plenty of room for both on the screen.
spree@example.com is great, but we don't want MikeAtYahooDotCom@hotmail.com
spilling out of the sidebar
@Sinetheta
Copy link
Contributor Author

Good eyes @tvdeyen, it's actually any margin bottom on the last item in the .main-content which would have caused that overflow, so I added a clearfix.

@tvdeyen
Copy link
Member

tvdeyen commented Dec 1, 2015

Perfect! :shipit: 🎉 💃

@jhawthorn
Copy link
Contributor

👍 Great! Let's get this in. Thank you @Mandily and @Sinetheta this looks great. Thanks @tvdeyen for the vigilant reviews, which improved this a lot.

@forkata
Copy link
Contributor

forkata commented Dec 14, 2015

I am very excited about this change 👍 Thanks for the awesome work @Sinetheta and @Mandily!

@tvdeyen tvdeyen mentioned this pull request Dec 14, 2015
@hhff
Copy link

hhff commented Dec 14, 2015

Ah, amazing and super extensible!! Love it / can't wait ❤️

@athal7
Copy link
Contributor

athal7 commented Dec 17, 2015

👍

@jordan-brough
Copy link
Contributor

👍 I tried this out on our codebase last night and was amazed at how few things we're broken! :) It'll probably take us a half day or something to work out the kinks on our site I'm guessing. Nice work and great job trying to preserve backward compatibility.

jhawthorn added a commit that referenced this pull request Dec 17, 2015
@jhawthorn jhawthorn merged commit 4aed3fe into solidusio:master Dec 17, 2015
@jhawthorn
Copy link
Contributor

Merged! 🎉

@tvdeyen
Copy link
Member

tvdeyen commented Dec 17, 2015

Booyaa!!! 🤘🏻

@Sinetheta
Copy link
Contributor Author

Thanks all, and big thanks to @tvdeyen without whom this would have been a much lamer UX with a lot more visual inconsistencies.

@mtomov
Copy link
Contributor

mtomov commented Dec 17, 2015

Thank you @Sinetheta , it is very hard to pull those large changes off, really great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add flyout triggered by hover to left nav Merge menu items