[WIP] Collapsible menu #1718

davidedistefano · 2017-02-17T16:57:34Z

This PR makes the Solidus admin more usable at small screen sizes.

At medium size devices, like small desktops or big tablets, the sidebar will collapse, leaving visible only the menu icons.

At mobile screen sizes instead, the menu will only be visible by clicking on the top right hamburger menu.

To make this PR works also on devices like smartphone or tablet the following line of code should be added inside the app <head> element:

<meta content="text/html; charset=UTF-8" http-equiv="Content-Type" />

The admin layout needs some tweaks to work fine with new sidebar. I suggest to use a fluid layout instead of the old Skeleton grid.

tvdeyen

Thanks for this. From a code point of view I only have some small change requests. I'm pretty pleased with how less code this feature is implemented. Kudos!

I would love to see a review from @Mandily on this.

tvdeyen · 2017-02-20T20:41:49Z

backend/app/assets/javascripts/spree/backend/navigation.coffee

+ if $(window).width() < 768
+ stickyElements.trigger 'sticky_kit:detach'
+ else
+ stickyElements.stick_in_parent(spacer: false)


I get very strange jumpy scrolling behaviour without the spacer.

Thanks for your feedback! Can you please tell me what browser you are using and at which window width do you see this jumpy scrolling behaviour.

Latest Chrome, small desktop viewport (~1024) lower the screen height so the sticky feature get's triggered.

it happens when page content is shorter than menu height, looking for a fix

also, it doesn't seem to be related to having the spacer or not, it happens even with it

@tvdeyen should be fixed with 8e9a39f, what do you think?

@kennyadsl I'm not sure about removing the sticky kit yet. @Sinetheta WDYT?

tvdeyen · 2017-02-20T20:42:34Z

backend/app/assets/javascripts/spree/backend/navigation.coffee

+checkSticky = ->
+ stickyElements = $('.admin-nav-sticky, .admin-nav')
+
+ if $(window).width() < 768


This should be <= 768 as the breakpoint starts at 768

tvdeyen · 2017-02-20T21:05:20Z

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

+ }
+ }
+}
+


IMO a small transition would enhance UX:

.admin-content, .admin-nav { transition: transform .2s ease-in; }

Actually we tried to add a CSS transition to the nav, but while using only CSS allows you to have a cleaner code, you don't have the same power as you have with Javascript :)

We found a bug in particular that occurs while resizing the window, as explained by this GIF:

So we thought that in order to avoid focusing too much on the transition effect, we'd implement a first version of the collapsible navigation without effect.

Any suggestion to avoid this bug?

I would not consider this a bug, but I also don't persist on this animation at all. It was just an idea for a little nicer UX.

tvdeyen · 2017-02-20T21:17:40Z

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

- }
+ > ul {
+ @include media($desktop) {
+ @include flyout;


IMO all breakpoints can profit from a flyout. Is there a particular reason you hide them from small viewports?

The flyout is triggered by hovering, so it doesn't actually work on a touch screen. it's more of a bonus for desktop users than a feature removed for mobile though. With a single tap they can still open the menu to access the lower levels.

A small screen does not equal a touch screen and vice versa. And this PR actually added more code to make this work "only" for desktop. That's why I proposed to remove this refactoring and say leave the flyout even for small screens.

tvdeyen · 2017-02-20T21:18:38Z

backend/app/assets/stylesheets/spree/backend/globals/_variables.scss

+
+// Media queries
+//--------------------------------------------------------------
+$desktop-width: 959px;


We should add !default here, so the variable can be overriden by shop devs.

graygilmore · 2017-02-20T23:03:35Z

backend/app/assets/stylesheets/spree/backend/globals/mixins/_media.scss

+/// }
+/// }
+
+@mixin media($media-query) {


I don't think this media query is necessary. It actually requires more characters than not having it:

.element { background: blue; // With mixin @include media($desktop) { background: green; } // Without mixin @media #{$desktop} { background: green } }

@graygilmore thanks for your feedback! We thought that having a mixin would be easier to add features to media query in future development, without updating code in the whole project. Also the mixin version seems to be a little more user friendly and more "readable".

But your suggestion also makes sense and we're open to change code in this direction if you want.

Good catch @graygilmore

@davidedistefano I don't think the added complexity of the mixin helps us here.

graygilmore

I don't see a way to toggle the collapseness. This was a (kind of hidden) requirement from the issue: Collapsible left nav #666 (comment)
I don't see labels when hovering the items that don't have a dropdown when the navigation is collapsed
the meta tag actually required is <meta name="viewport" content="width=device-width, initial-scale=1">
- this should probably be included as a part of this PR
the menu-button toggle on mobile stays in hover mode after tapping it (should always avoid using hover styles for mobile devices)

mtomov · 2017-03-24T14:04:33Z

backend/app/assets/stylesheets/spree/backend/globals/_variables.scss

+$large-device: "only screen and (min-width: #{$tablet-width})";
+$desktop: "only screen and (max-width: #{$desktop-width})";
+$tablet: "only screen and (max-width: #{$tablet-width})";
+$mobile: "only screen and (max-width: #{$mobile-width})";


The problem with this stuff is that you then need documentation for it. That's why something standardized (Bootstrap) should provide for all that boilerplate. For example, it's very elegant in Foundation:

https://foundation.zurb.com/sites/docs/media-queries.html#sass

@include breakpoint(medium) { // All CSS in here goes inside the media query }

If Bootstrap doesn't provide for such a mixin, then we can probably steel the one from Foundation, although it's unlikely that there isn't one such there.

@mtomov agree, but as long as we have skeleton layout I think we need to consider that exactly these breakpoints will trigger screen resizes. We just moved those into variables to be able to use them in all pages consistently.

Yeah, but Bootstrap is already part of Solidus, so all mixins it has should be available.

https://v4-alpha.getbootstrap.com/layout/overview/#responsive-breakpoints

Try the above ones, they should work:

@include media-breakpoint-up(xl) { ... }

sorry, I don't get it. This way it will use the bootstrap sizes for xl, lg, etc screen sizes but I think we need to use the skeleton ones which are defined here that could not match, right?

The link are pointing to is for the frontend.

And yes, hopefully they should match, otherwise we can't use Bootstrap ..

So, this must be the bootstrap configuration for the backend.

The skeleton one for the backend seems to have most of the values pretty fixed : ) .. so yeah, hopefully they match ..

Sorry for the wrong link. Cool, thanks, I didn't noticed those variables in backend. I'll give these mixins a try soon! :)

kennyadsl · 2017-03-24T17:15:57Z

I've implemented a basic collapse menu system, basically it adds a label in the menu that changes the checkbox value that shows the full menu, same as what the hamburger menu (only visible if <= tablet) does.

Collapsed

Open

Tablet

This PR still needs:

- reduce navigation width - hide the subnav on smaller screens and make it appear only on link hover - collapse the left menu only on mobile landscape

in order to avoid multiple lines and to fit right in the app header

this is needed to prevent scrolling or scrollbar to appear when the mobile menu is open

while scrolling when window height is small

- prevent jquery Sticky-kit plugin to create unnecessary divs - prevent admin nav content stick when scrolling on mobile - disable Sticky-kit on mobile - make admin nav scrollable on mobile

It seems like it's enough stable, at least for the backend: https://caniuse.com/#search=sticky Also, this change uniforms the admin styleguides which was using the js sticky version.

kennyadsl · 2017-10-04T12:23:42Z

closing this one since lot of layout things has changed and after working on this I think we should use a different approach (more JS driven).

tvdeyen requested changes Feb 20, 2017

View reviewed changes

graygilmore suggested changes Feb 20, 2017

View reviewed changes

kennyadsl force-pushed the collapsible-menu branch 2 times, most recently from a255271 to 8e9a39f Compare March 23, 2017 16:57

mtomov reviewed Mar 24, 2017

View reviewed changes

davidedistefano and others added 18 commits March 30, 2017 15:57

Add new mixin and variables for media queries

a9f1cf5

Improve navigation on smaller screens

82457df

- reduce navigation width - hide the subnav on smaller screens and make it appear only on link hover - collapse the left menu only on mobile landscape

Update admin layout media queries

8a7a5e2

Add mobile hamburger menu

713d5b7

Truncate breadcrumb text on small screen sizes

a055837

in order to avoid multiple lines and to fit right in the app header

Add new div containers to wrap admin content

f25b826

this is needed to prevent scrolling or scrollbar to appear when the mobile menu is open

Update admin flash messages layout

0c98fbe

Restore skeleton layout

c62d378

Add min-height to admin wrapper to prevent bounce effect

cb3e22f

while scrolling when window height is small

Fix admin scroll behavior

f135270

- prevent jquery Sticky-kit plugin to create unnecessary divs - prevent admin nav content stick when scrolling on mobile - disable Sticky-kit on mobile - make admin nav scrollable on mobile

Refactor collapsible menu code

a88bd99

Remove Bourbon deprecated mixins

3d89fee

Update header logo height

71eb52d

Remove media mixin

bb183a5

Remove sticky-kit js in favor of css sticky positioning

80c2e59

It seems like it's enough stable, at least for the backend: https://caniuse.com/#search=sticky Also, this change uniforms the admin styleguides which was using the js sticky version.

Make backend media variables overridable

d469d1c

Add a basic collapse menu system

4849f02

Switch to a Backbone.js version of navigation js

490330e

davidedistefano changed the title ~~Collapsible menu~~ [WIP] Collapsible menu Apr 6, 2017

Show flyout also on single-element menu items when collapsed

8a5aa54

kennyadsl force-pushed the collapsible-menu branch from 92c7e99 to 8a5aa54 Compare April 6, 2017 13:49

kennyadsl closed this Oct 4, 2017

kennyadsl mentioned this pull request Sep 24, 2018

Chore/make admin more responsive #2863

Closed

jacobherrington mentioned this pull request Nov 20, 2018

The navigation menu should be a collapsible #2961

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Collapsible menu #1718

[WIP] Collapsible menu #1718

davidedistefano commented Feb 17, 2017

tvdeyen left a comment

tvdeyen Feb 20, 2017

davidedistefano Feb 21, 2017

tvdeyen Feb 22, 2017

kennyadsl Mar 23, 2017

kennyadsl Mar 23, 2017

kennyadsl Mar 23, 2017

tvdeyen Mar 24, 2017

tvdeyen Feb 20, 2017

tvdeyen Feb 20, 2017

davidedistefano Feb 21, 2017

tvdeyen Feb 22, 2017

tvdeyen Feb 20, 2017

Mandily Feb 22, 2017

tvdeyen Feb 22, 2017 •

edited

Loading

tvdeyen Feb 20, 2017

kennyadsl Mar 23, 2017

graygilmore Feb 20, 2017

davidedistefano Feb 21, 2017

tvdeyen Feb 22, 2017

graygilmore Feb 22, 2017

graygilmore left a comment

mtomov Mar 24, 2017

kennyadsl Mar 24, 2017

mtomov Mar 24, 2017

kennyadsl Mar 24, 2017 •

edited

Loading

mtomov Mar 24, 2017 •

edited

Loading

kennyadsl Mar 24, 2017

mtomov Mar 24, 2017

kennyadsl commented Mar 24, 2017 •

edited

Loading

kennyadsl commented Oct 4, 2017

[WIP] Collapsible menu #1718

[WIP] Collapsible menu #1718

Conversation

davidedistefano commented Feb 17, 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

tvdeyen Feb 22, 2017 • edited Loading

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

graygilmore 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

kennyadsl Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

mtomov Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl commented Mar 24, 2017 • edited Loading

Collapsed

Open

Tablet

kennyadsl commented Oct 4, 2017

tvdeyen Feb 22, 2017 •

edited

Loading

kennyadsl Mar 24, 2017 •

edited

Loading

mtomov Mar 24, 2017 •

edited

Loading

kennyadsl commented Mar 24, 2017 •

edited

Loading