-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
</div> | ||
<nav class="admin-menu"> | ||
<ul data-hook="admin_tabs"> | ||
<%= render :partial => 'spree/admin/shared/tabs' %> |
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.
Since we're touching all of these files anyways I'd love for us to switch to the new hash syntax.
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.
No prob
NICEEE!!! 🎉 |
display: table; | ||
table-layout: fixed; | ||
a { | ||
display: inline-block; |
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.
If it's inline-block
and width: 100%
we should just be able to one-line this with display: block
.
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.
👍
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%); |
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.
If we care about Safari 8 (previous version) we'll need a mixin here.
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.
I always forget the prefixer.
5dac418
to
f3c9eb7
Compare
@@ -0,0 +1,5 @@ | |||
<div class='sidebar'> |
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.
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
.
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.
If there's only one main sidebar I'd prefer these to all just be sidebar-#{thing}
.
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.
I was just hedging my bets...
1531885
to
ea207d5
Compare
@@ -0,0 +1,5 @@ | |||
<div class="sidebar"> | |||
<%= render partial: 'spree/admin/shared/sidebar_left_header' %> |
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.
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
.
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.
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).
@Sinetheta it's already an improvement ;) BTW: In the settings menu, the right sub nav is somewhat misplaced: |
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. |
<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' %> |
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.
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
Thanks for pulling this down to test @tvdeyen!
|
c11911d
to
808f5cf
Compare
So, it looks like the remaining spec are failing due to overlap of the flash with what is now a higher content area.
|
#logo { height: 40px } | ||
img { | ||
vertical-align: middle; | ||
margin-top: -12px; // hand-tweaking because the solid logo is bottom heavy |
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.
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.
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.
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.
@jhawthorn I agree. Good point. |
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.
[email protected] is great, but we don't want [email protected] spilling out of the sidebar
Good eyes @tvdeyen, it's actually any margin bottom on the last item in the |
Perfect! 🎉 💃 |
👍 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. |
I am very excited about this change 👍 Thanks for the awesome work @Sinetheta and @Mandily! |
Ah, amazing and super extensible!! Love it / can't wait ❤️ |
👍 |
👍 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. |
Merged! 🎉 |
Booyaa!!! 🤘🏻 |
Thanks all, and big thanks to @tvdeyen without whom this would have been a much lamer UX with a lot more visual inconsistencies. |
Thank you @Sinetheta , it is very hard to pull those large changes off, really great! |
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