Make the sidebar menu collapsible #5

davidedistefano · 2017-02-06T17:58:45Z

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.

kennyadsl

Just small changes proposed but 👍

kennyadsl · 2017-02-06T20:37:10Z

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

+    }
+  }
+}
+


@davidedistefano maybe we can nest the last 2 elements inside the &:checked ~ .admin-wrapper one, something like:

#menu-button { display: none; &:checked ~ .admin-wrapper { overflow: hidden; .admin-nav { // ... } .admin-content { // ... } } }

kennyadsl · 2017-02-06T21:06:37Z

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

@davidedistefano I find this part is a bit confusing (probably it already was) and I'd take advantage of this change to make a small refactor:

.admin-nav-menu { ul { margin: 0; padding: 0; list-style: none; } li { background: $color-navbar-bg; &:hover { background: $color-navbar-active-bg; > a { color: $color-navbar-active; } } // flyout nav > ul { position: absolute; top: 0; left: 100%; width: $width-sidebar-flyout; margin-left: 0; border: $border-sidebar; background: $color-navbar-submenu-bg; box-shadow: $box-shadow; @include caret($direction: left, $color-caret: $color-navbar-submenu-bg); @include media($tablet) { position: relative; left: 0; width: $width-sidebar; border-top: 0; border-bottom: 0; } &:before { z-index: 1; top: 1.5em; } } &.selected { background: $color-navbar-active-bg; @include media($desktop) { position: relative; } &:not(:hover) > ul { @include media($desktop) { display: none; } @include media($tablet) { display: block; } } li { padding-left: 2em; } > a { color: $color-navbar-active; } } &:not(.selected) { position: relative; > ul { @include media($tablet) { display: none; } } &:not(:hover) > ul { display: none; } } }

Since the flyout stuff is used just once, we can remove the flyout mixin.

kennyadsl · 2017-02-06T21:07:53Z

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

@@ -0,0 +1,16 @@
+@mixin flyout {


as said above, I'd remove this mixin in favor of a better code organization.

kennyadsl · 2017-02-06T21:09:18Z

backend/app/views/spree/layouts/admin.html.erb

-    <%= render "spree/admin/shared/spinner" %>
+    <input type="checkbox" id="menu-button" />
+
+    <div class="admin-wrapper">


I'd remove the empty line under this div

kennyadsl · 2017-02-06T21:18:06Z

@davidedistefano also, we can anticipate this PR removing all bourbon browser prefixer mixins from the new code we are writing

graygilmore · 2017-02-06T21:42:53Z

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

@@ -28,6 +28,12 @@
 .flash-wrapper {
  @include position(fixed, null 0 0 $width-sidebar);
  z-index: 1000;
+  @include media($desktop) {
+    @include position(fixed, null 0 0 $width-sidebar-collapsed);


This can just be left: $width-sidebar-collapsed.

graygilmore · 2017-02-06T21:43:05Z

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

+    @include position(fixed, null 0 0 $width-sidebar-collapsed);
+  }
+  @include media($desktop) {
+    @include position(fixed, null 0 0 0);


Similarly this can just be left: 0.

graygilmore · 2017-02-06T21:45:25Z

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

+  @include media($tablet) {
+    width: $width-sidebar;
+    position: fixed;
+    margin: 0 0 0 -200px;


Should this be -$width-sidebar?

Also would prefer to use translateX() to move it over but not too concerned about the margin.

graygilmore · 2017-02-06T21:48:15Z

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

+      display: none;
+    }
+    @include media($tablet) {
+      display: inline-block;


If it's inline-block by default do we need this additional media query?

Since it is not mobile first approach, we need to reset the display property.

graygilmore · 2017-02-06T21:52:00Z

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})";


Prefer always going up for media queries (making sure to always use min-width). This helps create a mobile first approach:

$tablet: "screen and (min-width: 481px)"; $desktop: "screen and (min-width: 769px)"; .thing { // default styles + mobile style: style; @media $tablet { // more things } @media $desktop { // more things } }

@graygilmore for now we just converted the skeleton media queries set here. We internally discussed about changing them to a mobile first approach but it seemed like something not related to this PR and probably something that need a better testing.

👍 I also only just realized this wasn't a PR into Solidus itself. 🤦‍♂️

it will be soon so a review at this stage is very helpful, thanks @graygilmore

mtylty

I'm not an expert on stylesheets but the CoffeeScript code looks good to me 👍

Mandily · 2017-02-07T17:41:45Z

This looks great! I know it's not exactly part of this scope of work, but I was wondering if you could make the admin-nav-header class have a height of 47px? That should make the white solidus logo area the same height as the blue bar beside it so everything is aligned.

- 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

davidedistefano · 2017-02-17T16:56:46Z

will be reopened on solidus repository

Add support for refunding payments

davidedistefano requested review from mtylty, kennyadsl and aleph1ow February 6, 2017 17:59

kennyadsl requested changes Feb 6, 2017

View reviewed changes

graygilmore suggested changes Feb 6, 2017

View reviewed changes

mtylty approved these changes Feb 7, 2017

View reviewed changes

davidedistefano added 10 commits February 17, 2017 17:43

Add new mixin and variables for media queries

27bdef1

Improve navigation on smaller screens

55a168d

- 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

51c5893

Add mobile hamburger menu

2d112fa

Truncate breadcrumb text on small screen sizes

0623804

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

Add new div containers to wrap admin content

321cdac

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

Update admin flash messages layout

b9d3fd0

Restore skeleton layout

8ecde31

Add min-height to admin wrapper to prevent bounce effect

14f07bc

while scrolling when window height is small

Fix admin scroll behavior

ed5f03f

- 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

davidedistefano force-pushed the collapsible-menu branch from fd941b8 to 9b2f3e8 Compare February 17, 2017 16:52

davidedistefano added 3 commits February 17, 2017 17:54

Refactor collapsible menu code

69eb16a

Remove Bourbon deprecated mixins

9e6ba1a

Update header logo height

f43ceb7

davidedistefano force-pushed the collapsible-menu branch from 9b2f3e8 to f43ceb7 Compare February 17, 2017 16:55

davidedistefano closed this Feb 17, 2017

spaghetticode pushed a commit that referenced this pull request Mar 26, 2021

Merge pull request #5 from adammathys/refund

fa7f4f7

Add support for refunding payments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the sidebar menu collapsible #5

Make the sidebar menu collapsible #5

davidedistefano commented Feb 6, 2017

kennyadsl left a comment

kennyadsl Feb 6, 2017

kennyadsl Feb 6, 2017

kennyadsl Feb 6, 2017

kennyadsl Feb 6, 2017

kennyadsl commented Feb 6, 2017 •

edited

Loading

graygilmore Feb 6, 2017

graygilmore Feb 6, 2017

graygilmore Feb 6, 2017

graygilmore Feb 6, 2017

graygilmore Feb 6, 2017

davidedistefano Feb 7, 2017

graygilmore Feb 6, 2017

kennyadsl Feb 6, 2017

graygilmore Feb 6, 2017

kennyadsl Feb 6, 2017

mtylty left a comment

Mandily commented Feb 7, 2017

davidedistefano commented Feb 17, 2017

Make the sidebar menu collapsible #5

Make the sidebar menu collapsible #5

Conversation

davidedistefano commented Feb 6, 2017

kennyadsl 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

kennyadsl commented Feb 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtylty left a comment

Choose a reason for hiding this comment

Mandily commented Feb 7, 2017

davidedistefano commented Feb 17, 2017

kennyadsl commented Feb 6, 2017 •

edited

Loading