[4.0] Sidebar - switch to vertical collapsing #22587

ciar4n · 2018-10-11T15:57:00Z

Pull Request for Issue # .

Summary of Changes

Redesign of the sidebar with vertically collapsing/expanding menu.

Testing instructions

Can't be tested with patchtester.

Documentation Changes Required

houndci-bot · 2018-10-11T15:57:13Z

administrator/templates/atum/scss/blocks/_sidebar.scss

-  opacity: 1 !important;
-}
+
+}


Files should end with a trailing newline

houndci-bot · 2018-10-11T15:57:14Z

administrator/templates/atum/scss/blocks/_sidebar.scss

-  visibility: visible !important;
-  opacity: 1 !important;
-}
+


Line contains trailing whitespace

houndci-bot · 2018-10-11T15:57:14Z

administrator/templates/atum/scss/blocks/_sidebar.scss

-    transition-delay: 0s !important;
+  .sidebar-item-title,
+  .has-arrow::after  {
+    display: none


Declaration should be terminated by a semicolon

houndci-bot · 2018-10-11T15:57:15Z

administrator/templates/atum/scss/blocks/_sidebar.scss

-    visibility: hidden;
-    transition-delay: 0s !important;
+  .sidebar-item-title,
+  .has-arrow::after  {


Opening curly brace { should be preceded by one space

houndci-bot · 2018-10-11T15:57:15Z

administrator/templates/atum/scss/blocks/_sidebar.scss

+      transform: rotate(-135deg) translate(0, -50%);
+    }
+  }
+


Line contains trailing whitespace

houndci-bot · 2018-10-11T15:57:16Z

administrator/templates/atum/scss/blocks/_sidebar.scss

-      cursor: text;
-      background: none;
+    a {
+      padding: 4px 5px 4px 2px;


Properties should be ordered display, padding

houndci-bot · 2018-10-11T15:57:17Z

administrator/templates/atum/scss/blocks/_sidebar.scss

    }
+  }
+


Line contains trailing whitespace

houndci-bot · 2018-10-11T15:57:17Z

administrator/templates/atum/scss/blocks/_sidebar.scss

-        outline: 0;
-        box-shadow: inset 0 0 0 1px #78aeda;
+      &::before {
+        content: '';


Properties should be ordered position, top, bottom, left, width, content, background-color
Prefer double-quoted strings

houndci-bot · 2018-10-11T15:57:17Z

administrator/templates/atum/scss/blocks/_sidebar.scss

+      background-color: $sidebar-sub-bg;
+    }
+    > a {
+      padding: 10px 6px;


Properties should be ordered display, padding

houndci-bot · 2018-10-11T15:57:18Z

administrator/templates/atum/scss/blocks/_sidebar.scss

-    &:first-of-type {
-      border-top: 1px solid rgba($black, .15);
+    &:hover {
+      color: white;


Color white should be written in hexadecimal form as #ffffff

houndci-bot · 2018-10-11T16:01:15Z

administrator/templates/atum/scss/blocks/_sidebar.scss

+        bottom: 0;
+        left: 0;
+        width: 3px;
+        content: '';


Prefer double-quoted strings

houndci-bot · 2018-10-11T16:01:15Z

administrator/templates/atum/scss/blocks/_sidebar.scss

  opacity: 1;
+


Line contains trailing whitespace

brianteeman · 2018-10-11T16:05:44Z

Does this mean the quick link for New Item has been abandoned?

ciar4n · 2018-10-11T16:18:33Z

I have no idea. I haven't been part of any conversation on the topic. I'm guessing if there is no one to implement, it probably is.

brianteeman · 2018-10-11T16:23:16Z

So now I am confused because I thought that the only problem with the existing menu was that it didnt support that

brianteeman · 2018-10-11T16:39:52Z

When used with the menu bar toggled to collapsed

menu icons look very small to me (I am not a designer)
menu links that have a submenu do not expand - only the direct menu items eg system
when you select an item eg users->groups then on page load the menu is collapsed and there is no indicator what is the active menu item

From an accessibility perspective

there needs to be greater background colour contrast between the normal and hover states at the top level
there needs to be a background color on the submenu hover state and it looks like there is a subtle text color change on the hover which depending on the background might need tweaking
the font icon needs to be aria-hidden
the parent items have a role of menuitem which iirc is not correct as they are not active links and should have a role=presentation
the parent items should have an aria-haspopup = true

simbus82 · 2018-10-11T16:44:09Z

Very nice! But i also feel the lack of "New Item"...
Another consideration on the UX: the switch is too similar to the menu items. It should be smaller or less highlighted.

ciar4n · 2018-10-11T17:26:21Z

So now I am confused because I thought that the only problem with the existing menu was that it didn't support that

I honestly can't comment on that.. it wasn't from me you heard it. My reasoning for changing this to an accordion is I find the current menu gimmicky at best. Imo accordions work in almost every instance (device, number of items, expected behavior, expandable, industry standard ... ). It is fair to say we have tried everything else.

menu icons look very small to me (I am not a designer)

Personally, I have always found the icons too big in J4. I'll increase the size slightly although I would like to avoid increasing the height of each menu item. A downside to accordions is the need to scroll on larger menus, increased menu item heights would increase this probability.

menu links that have a submenu do not expand - only the direct menu items eg system

I presume you are referring to when an item is opened? The menu should stay open on the 'current' menu item?

I'll update the PR regarding the accessibility issues presuming I get any indication that this is deemed an improvement and might actually get merged.

Regarding the 'new item' option there is no reason why it can't be added to this menu after. There are both a11y and PHP considerations on how best it should be done. Neither of which are areas I am skilled in.

brianteeman · 2018-10-11T17:34:20Z

menu icons look very small to me (I am not a designer)

Personally, I have always found the icons too big in J4. I'll increase the size slightly although I would like to avoid increasing the height of each menu item. A downside to accordions is the need to scroll on larger menus, increased menu item heights would increase this probability.

Point taken about the menu item height

menu links that have a submenu do not expand - only the direct menu items eg system

I presume you are referring to when an item is opened? The menu should stay open on the 'current' menu item?

Sorry something got lost as I was editing the report. There are two different issues

The menu should stay open on the 'current' menu item?
The menu doesn't expand when collapsed to icon only

ciar4n · 2018-10-12T00:01:45Z

Do we really need the option to toggle the menu down to just the icons? It complicates this no end and also bordering on being a bit of a gimmick.

brianteeman · 2018-10-12T07:56:56Z

Personally it is essential. For me it was the thing that convinced me a vertical menu was a good choice. In all the time I have been working with J4 I have not had it expanded at all. As for it being a gimmick it is a very common design pattern.

houndci-bot · 2018-10-12T08:39:11Z

administrator/templates/atum/scss/blocks/_sidebar.scss

-    text-align: center;
-    vertical-align: middle;
+  .menuitem-group {
+    text-transform: uppercase;


Properties should be ordered margin-top, font-size, text-transform, letter-spacing, list-style

C-Lodder · 2018-10-12T08:41:58Z

build/media/mod_menu/js/admin-menu.js

@@ -124,9 +126,10 @@ Joomla = window.Joomla || {};
 				allLinks[i].classList.add('active');
 				// Auto Expand First Level
 				if (!allLinks[i].parentNode.classList.contains('parent')) {
-					mainNav.classList.add('child-open');
+					mainNav.classList.add('CIARAN');


😄 Lucky really.. they are usually a lot more offensive

C-Lodder · 2018-10-12T08:52:52Z

My thoughts are with you Ciaran. Probably the 6th time you've revisited this and it's now back to square 1 :D

Much better using an accordion:

Easier to maintain
Less code
Works flawlessly on all device sizes
Simple
No dirty work arounds required when too many items are there
Easier to make accessible

One thing I will say it just ensure that when you have an expanded menu item, ensure you can still scroll through the menu if there are too many items.

brianteeman · 2018-10-12T08:56:44Z

current and toggle now working as expected although I would add some styling to <a active similar to the styling on the active parent

.main-nav>li.active>a {
    background-color: rgba(0,0,0,0.4);
}

there are still a lot of a11y errors

ciar4n · 2018-10-12T09:38:09Z

@C-Lodder You have said accordions are the way to go from day one... we should have listened 😄

houndci-bot · 2018-10-12T09:38:15Z

administrator/templates/atum/scss/blocks/_sidebar.scss

-      letter-spacing: 1px;
-    }
+  .divider {
+    background-color: $sidebar-text-color;


Properties should be ordered height, margin, list-style, background-color, opacity

houndci-bot · 2018-10-12T09:38:15Z

administrator/templates/atum/scss/blocks/_sidebar.scss


-    a::before {
-      display: none;
+        &::before {


Nesting should be no greater than 4, but was 5

houndci-bot · 2018-10-12T09:38:15Z

administrator/templates/atum/scss/blocks/_sidebar.scss

-    > span {
-      color: rgba($white, .4);
+      &::before {
+        font-family: 'FontAwesome';


Properties should be ordered position, top, left, font-family, font-size, color, content
Prefer double-quoted strings

houndci-bot · 2018-10-12T09:38:15Z

administrator/templates/atum/scss/blocks/_sidebar.scss


      &:hover {
+        background-color: rgba(0,0,0,.4);


Properties should be ordered color, background-color

ciar4n · 2018-10-12T09:51:16Z

@brianteeman

I have amended the styling per your a11y suggestions.

the font icon needs to be aria-hidden

the parent items have a role of menuitem which iirc is not correct as they are not active links and should have a role=presentation

the parent items should have an aria-haspopup = true

The above I am not really sure where/how to change. Could you possibly create issues and hopefully someone with the smarts can fix them. The issues are present regardless of this PR.

laoneo · 2018-10-16T13:54:04Z

I have tested this item ✅ successfully on c053ed5

The only issue I found was that when clicking on a component subitem, it doesn't expand to it on a page reload. But this can be fixed then in a followup pr.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22587.}

…to metismenujs

ciar4n · 2018-10-16T14:04:32Z

Second levels will now open if active

laoneo · 2018-10-16T14:07:22Z

I have tested this item ✅ successfully on d69c02c

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22587.}

brianteeman · 2018-10-16T14:16:02Z

LGTM lets merge and then I can fix the a11y

brianteeman · 2018-10-16T15:24:14Z

administrator/templates/atum/scss/blocks/_sidebar.scss

@@ -136,10 +137,17 @@
    }
  }

+  .divider,
+  .menuitem-group {


shouldnt this be just menuitem not menitem-group

This is the separator element as seen under Menus ('Site'). Class name is there since Joomla 3. Shall I change it? Not sure 'menuitem' is suitable.

oops commented in the main thread

brianteeman · 2018-10-16T15:25:31Z

small css change required https://github.com/joomla/joomla-cms/pull/22587/files#r225589673

brianteeman · 2018-10-16T15:39:22Z

ah - ok - missed that. The issue I wanted to resolve was the circle bullet. You successfully used a background image but the list-style is still there

ciar4n · 2018-10-16T16:03:35Z

You can ignore that. Chrome lists default values set by the browser as 'user'. That CSS is not in the template.

brianteeman · 2018-10-16T16:10:25Z

ok - maybe it is something else causing the error I am hearing with a11y - I will look into that when I do the a11y pr

C-Lodder · 2018-10-16T16:17:32Z

Is the screenreader reading something it shouldnt?

brianteeman · 2018-10-16T16:20:22Z

yes - it is saying white bullet

brianteeman · 2018-10-17T15:51:39Z

I have tested this item ✅ successfully on d69c02c

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22587.}

brianteeman · 2018-10-17T15:52:06Z

PR ready to submit to address accessibility once this is merged

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/22587.}

laoneo · 2018-10-18T05:04:38Z

Thanks, makes working with components usable.

ciar4n · 2018-10-18T07:39:48Z

@brianteeman @laoneo @C-Lodder Thanks for the help on this

[4.0] Sidebar redo

1b28884

joomla-cms-bot added the PR-4.0-dev label Oct 11, 2018

houndci-bot reviewed Oct 11, 2018

View reviewed changes

happy hound

c48a0e8

houndci-bot reviewed Oct 11, 2018

View reviewed changes

more hound

3129b30

ciar4n closed this Oct 11, 2018

ciar4n reopened this Oct 11, 2018

Fix current and toggle

86f02c2

houndci-bot reviewed Oct 12, 2018

View reviewed changes

ciar4n changed the title ~~[4.0] Sidebar redo~~ [4.0] Sidebar - switch to vertical collapsing Oct 12, 2018

C-Lodder reviewed Oct 12, 2018

View reviewed changes

ciar4n added 2 commits October 12, 2018 09:44

Increase parent hover contrast

85eccfc

cs

1e524d0

a11y styling

52decbe

houndci-bot reviewed Oct 12, 2018

View reviewed changes

happy hound

c85453d

joomla-cms-bot removed the Language Change This is for Translators label Oct 16, 2018

ciar4n and others added 5 commits October 16, 2018 11:26

conflicts

7b4cc7d

cs

9d920f0

Merge branch '4.0-dev' into metismenujs

cc01498

Update default.php

2d5b270

Merge branch '4.0-dev' into metismenujs

c053ed5

ciar4n added 2 commits October 16, 2018 14:59

Open second level if active

9d68fff

Merge branch 'metismenujs' of https://github.com/ciar4n/joomla-cms in…

d69c02c

…to metismenujs

brianteeman reviewed Oct 16, 2018

View reviewed changes

zero-24 removed their request for review October 16, 2018 18:05

laoneo merged commit 294cdb3 into joomla:4.0-dev Oct 18, 2018

laoneo added this to the Joomla 4.0 milestone Oct 18, 2018

laoneo mentioned this pull request Oct 18, 2018

[4.0] [RFC] The component sidebar #16020

Closed

ciar4n deleted the metismenujs branch October 18, 2018 07:39

ciar4n mentioned this pull request Sep 12, 2019

[4.0] metismenu upgrade #26275

Merged

ciar4n mentioned this pull request May 11, 2020

[4.0] Template Atum closed sidebar hover fix #29023

Merged

[4.0] Sidebar - switch to vertical collapsing #22587

[4.0] Sidebar - switch to vertical collapsing #22587

Conversation

ciar4n commented Oct 11, 2018 • edited Loading

Summary of Changes

Testing instructions

Documentation Changes Required

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

brianteeman commented Oct 11, 2018

ciar4n commented Oct 11, 2018

brianteeman commented Oct 11, 2018

brianteeman commented Oct 11, 2018 • edited Loading

simbus82 commented Oct 11, 2018 • edited Loading

ciar4n commented Oct 11, 2018 • edited Loading

brianteeman commented Oct 11, 2018

ciar4n commented Oct 12, 2018

brianteeman commented Oct 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C-Lodder commented Oct 12, 2018 • edited Loading

brianteeman commented Oct 12, 2018

ciar4n commented Oct 12, 2018

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

ciar4n commented Oct 12, 2018

laoneo commented Oct 16, 2018

ciar4n commented Oct 16, 2018

laoneo commented Oct 16, 2018

brianteeman commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianteeman commented Oct 16, 2018

brianteeman commented Oct 16, 2018

ciar4n commented Oct 16, 2018

brianteeman commented Oct 16, 2018

C-Lodder commented Oct 16, 2018

brianteeman commented Oct 16, 2018

brianteeman commented Oct 17, 2018

brianteeman commented Oct 17, 2018

laoneo commented Oct 18, 2018

ciar4n commented Oct 18, 2018

ciar4n commented Oct 11, 2018 •

edited

Loading

brianteeman commented Oct 11, 2018 •

edited

Loading

simbus82 commented Oct 11, 2018 •

edited

Loading

ciar4n commented Oct 11, 2018 •

edited

Loading

C-Lodder commented Oct 12, 2018 •

edited

Loading