Problem/Motivation

Main menu fills all height of window on smartphones up to 460px width.
See screenshot below.
Current menu screenshot

Proposed solution

Make the main menu collapsible and give possibility to expand/collapse it.
Almost the same solution was implemented and commited to the Responsive Bartik D7 theme. See #1873156: Make menu collapsible on small screen resolutions
Please see screenshots below.
Collapsed:
Collapsed menu
Expanded:
Expanded menu

How to test

This section explain the recommended way to test:

  1. Apply the patch
  2. Clear all caches
  3. Using Google Chrome emulation to test it on one mobile device
  4. goole chrome emulation

Related issues

Remaining tasks

Review the patch

CommentFileSizeAuthor
#120 bartik-made-menu-collapsible-1880488-119.patch4.02 KBLewisNyman
#120 interdiff.txt644 bytesLewisNyman
#118 Screenshot 2014-06-01 12.17.06.jpg67.53 KBLewisNyman
#118 bartik-made-menu-collapsible-1880488-118.patch4.02 KBLewisNyman
#118 interdiff.txt2.77 KBLewisNyman
#114 Test-Menu.png216.92 KBAmad Tababa
#112 Test Menu.png216.92 KBAmad Tababa
#110 Emulation-Google-chrom.png236.5 KBAmad Tababa
#102 menu-extended.png41.46 KBrteijeiro
#102 menu-collapsed.jpg30.23 KBrteijeiro
#99 interdiff-1880488-92-98.txt1.8 KBKonstantin Komelin
#99 bartik-made-menu-collapsible-1880488-98.patch4.22 KBKonstantin Komelin
#92 interdiff-1880488-85-92.txt766 bytesKonstantin Komelin
#92 bartik-made-menu-collapsible-1880488-92.patch5.71 KBKonstantin Komelin
#85 bartik-1880488-85.patch5.44 KBrteijeiro
#73 bartik-1880488-73.patch6.5 KBdjroshi
#65 chart.png1.19 KBKonstantin Komelin
#56 bartik-1880488-56.patch6.5 KBdjroshi
#56 interdiff.txt354 bytesdjroshi
#55 menu-test-ie9-10.jpg24.03 KBmjohnq3
#54 bartik-1880488-54.patch6.52 KBdjroshi
#54 interdiff.txt1.94 KBdjroshi
#52 bartik-1880488-52.patch6.18 KBjibran
#52 interdiff.txt518 bytesjibran
#40 bartik-1880488-40.patch6.16 KBdjroshi
#36 bartik-1880488-36.patch5.12 KBdjroshi
#33 bartik-1880488-33.patch5.22 KBdjroshi
#32 1880488-margins.png1.15 KBKonstantin Komelin
#30 bartik-1880488-30.patch5.1 KBdjroshi
#26 bartik-1880488-26.patch5.1 KBdjroshi
#25 bartik-1880488-25.patch3.82 KBdjroshi
#24 bartik-1880488-24.patch5.96 KBdjroshi
#20 bartik-1880488-20.patch4.65 KBKonstantin Komelin
#12 bartik-1880488-12.patch4.97 KBKonstantin Komelin
#10 bartik-1880488-10.patch4.65 KBKonstantin Komelin
#8 bartik-1880488-8.patch4.64 KBKonstantin Komelin
#1 bartik-1880488-1.patch5.25 KBKonstantin Komelin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Konstantin Komelin’s picture

FileSize
5.25 KB

Patch attached.

webchick’s picture

Issue tags: +mobile

Tagging with Mobile to get this on the right peoples' radars.

Konstantin Komelin’s picture

Okay, thanks!

Konstantin Komelin’s picture

Had problem with RTL language demo but now it's working.

nod_’s picture

Status: Active » Needs work

See how it's already done in vertical-tabs.js resize event is expensive to use and that's probably not worth it. For now it only checks at page load.

Konstantin Komelin’s picture

Thanks, nod_. I'll look at vertical tabs approach.

nod_’s picture

Also try to use <button> that's the right element for the job, not an empty <a> :) some examples at #1719640: Use 'button' element instead of empty links

Konstantin Komelin’s picture

Status: Needs work » Needs review
FileSize
4.64 KB

I've done improvements according to #5 and #7.
- It checks client width at page load only (wo resize event handling)
- Replaced <a> with <button>

Attached new patch and updated the demo http://d8.komelin.com.

One issue I see now is menu hides on portrait <-> landscape switching.
Any ideas?

nod_’s picture

can you use .on('click', function… instead of .click(function… ?

Konstantin Komelin’s picture

FileSize
4.65 KB

Replaced .click with .on('click'. Updated demo.

rteijeiro’s picture

Status: Needs review » Needs work

I am afraid the patch is corrupted by a png image.

Please, review it.

Konstantin Komelin’s picture

FileSize
4.97 KB

Re-created patch with command:
git diff --full-index --binary

Checked with command:
git apply -v --check bartik-1880488-12.patch
Results:

Checking patch core/themes/bartik/bartik.info...
Checking patch core/themes/bartik/css/style.css...
Checking patch core/themes/bartik/images/toggle.png...
Checking patch core/themes/bartik/js/collapsible-menu.js...
Checking patch core/themes/bartik/template.php...
Konstantin Komelin’s picture

Status: Needs work » Needs review
rteijeiro’s picture

Status: Needs review » Needs work

Sorry but the patch gives me errors when applying:

$ git apply bartik-1880488-12.patch 
bartik-1880488-12.patch:71: trailing whitespace.
?PNG
error: patch failed: core/themes/bartik/bartik.info:9
error: core/themes/bartik/bartik.info: patch does not apply
error: patch failed: core/themes/bartik/css/style.css:1520
error: core/themes/bartik/css/style.css: patch does not apply
error: core/themes/bartik/images/toggle.png: already exists in working directory
error: core/themes/bartik/js/collapsible-menu.js: already exists in working directory
error: patch failed: core/themes/bartik/template.php:11
error: core/themes/bartik/template.php: patch does not apply

Could someone test it too?

I am trying to see what's happening.

mjohnq3’s picture

Status: Needs work » Needs review

The above patch applied cleanly for me except for the whitespace error.

$ git apply -v bartik-1880488-12.patch
bartik-1880488-12.patch:71: trailing whitespace.
89PNG
Checking patch core/themes/bartik/bartik.info...
Checking patch core/themes/bartik/css/style.css...
Checking patch core/themes/bartik/images/toggle.png...
Checking patch core/themes/bartik/js/collapsible-menu.js...
Checking patch core/themes/bartik/template.php...
Applied patch core/themes/bartik/bartik.info cleanly.
Applied patch core/themes/bartik/css/style.css cleanly.
Applied patch core/themes/bartik/images/toggle.png cleanly.
Applied patch core/themes/bartik/js/collapsible-menu.js cleanly.
Applied patch core/themes/bartik/template.php cleanly.
warning: 1 line adds whitespace errors.
mbrett5062’s picture

I am wondering if we can take a similar approach to the collapsed menu button as Twitter Bootstrap.
Replace the PNG image with an HTML button and some simple CSS.

HTML

<button class="btn btn-navbar" data-target=".nav-collapse" data-toggle="collapse" type="button">
<span class="icon-bar"></span>
<span class="icon-bar"></span>
<span class="icon-bar"></span>
</button>

CSS

.navbar .btn-navbar {
    background-color: #EDEDED;
    background-image: linear-gradient(to bottom, #F2F2F2, #E5E5E5);
    background-repeat: repeat-x;
    border-color: rgba(0, 0, 0, 0.1) rgba(0, 0, 0, 0.1) rgba(0, 0, 0, 0.25);
    box-shadow: 0 1px 0 rgba(255, 255, 255, 0.1) inset, 0 1px 0 rgba(255, 255, 255, 0.075);
    color: #FFFFFF;
    display: none; // removed on breakpoint for small screens
    float: right;
    margin-left: 5px;
    margin-right: 5px;
    padding: 7px 10px;
    text-shadow: 0 -1px 0 rgba(0, 0, 0, 0.25);
}

.navbar .btn-navbar .icon-bar {
    background-color: #F5F5F5;
    border-radius: 1px 1px 1px 1px;
    box-shadow: 0 1px 0 rgba(0, 0, 0, 0.25);
    display: block;
    height: 2px;
    width: 18px;
}

.nav-collapse, .nav-collapse.collapse {
    height: 0;
    overflow: hidden;
}
.navbar .btn-navbar {
    display: block;
}

.btn {
    -moz-border-bottom-colors: none;
    -moz-border-left-colors: none;
    -moz-border-right-colors: none;
    -moz-border-top-colors: none;
    border-image: none;
    border-radius: 4px 4px 4px 4px;
    border-style: solid;
    border-width: 1px;
    cursor: pointer;
    font-size: 14px;
    line-height: 20px;
    margin-bottom: 0;
    text-align: center;
    vertical-align: middle;
}

This would allow the button to be styled if the user wishes to override defaults.

rteijeiro’s picture

Status: Needs review » Needs work

Hi, I finally applied #12 patch, removing the collapsible-menu.js and toggle.png files, but now the menu completely disappeared.

Should I have to enable or config some modules?

Konstantin Komelin’s picture

Status: Needs work » Needs review

No, you shouldn't. You should clean up your source base from the patch changes, pull latest changes from 8.x branch, and then apply patch again (if you wish).

rteijeiro’s picture

Tried again to test but no luck.

My menu disappears after patch apply. I will see what is happening.

Maybe someone could test it too.

Konstantin Komelin’s picture

FileSize
4.65 KB

Re-created the patch using git diff --binary.

It does work. I've applied it to the demo site sources http://d8.komelin.com/.

Konstantin Komelin’s picture

Status: Needs review » Needs work
Issue tags: -mobile

The last submitted patch, bartik-1880488-20.patch, failed testing.

Konstantin Komelin’s picture

Status: Needs work » Needs review
Issue tags: +mobile

#20: bartik-1880488-20.patch queued for re-testing.

djroshi’s picture

FileSize
5.96 KB

For your consideration, this is my attempt at a css-only implementation of this design, which I threw together as part of the drupalcon sydney code sprint.

djroshi’s picture

FileSize
3.82 KB

I have revisited the css and tidied it up a lot, removing superfluous css and a bunch of unneccessary body:not(:target) identifiers - leaving the minimum required to ensure that this functions well in browsers that support :target and degrades nicely in those that don't...

djroshi’s picture

FileSize
5.1 KB

Looks like toggle.png was omitted from the last patch, otherwise this patch is the same as #25

mjohnq3’s picture

Status: Needs review » Needs work

I applied the latest patch on my local installation of D8 and also on a sandbox on http://simplytest.me/. In both instances at the screen size that triggers the collapsible menu, it does not toggle. Clicking on the menu button will open the menu but it cannot be closed.

djroshi’s picture

I haven't used http://simplytest.me/ before - I can see that being quite useful, thanks.

I think back in #24 I had it so clicking anywhere that wasn't a menu item would close the menu. In #26 the menu only closes if you click the menu button. I don't really mind either way (as long as it works, of course) so please let me know which behavior you think is best...

I tested #26 against Drupal Core 8.x on http://simplytest.me/ and it seems to be working OK for me. So far tested in Safari, FF, Chrome.

mjohnq3’s picture

Status: Needs work » Needs review

I reapplied the patch in #26 on a http://simplytest.me/ sandbox and found that clicking on the menu button will open it but the menu can only be closed by clicking on the menu area not the button.

We need a few other people to test this - both approaches - the javascript version and the css only one.

I personally have no particular favorite. Both appear to work, are sound, and are generally in use, although the js is probably more common.

djroshi’s picture

FileSize
5.1 KB

I have reviewed #20 and I can confirm that it does work - however on a device with javascript disabled the menu disappears completely.

I am hoping this patch will fix the issue with the button not triggering menu close.

mjohnq3’s picture

Yes, great, the menu works fine now with the above patch.

If we use the javascript approach, the menu would need to shown as is, not hidden, if js were not enabled on the device. I believe the OP was aware of that.

Konstantin Komelin’s picture

FileSize
1.15 KB

Tested #30. It works great. Thanks djroshi!

Advantages of CSS approach (#30):
- It works with disabled JS
- It works with changing of device orientation Portrait <-> Landscape
- We don't have to load 'drupal' and 'jquery' libraries for every page

Advantage of JS approach (#20):
- The toggle effect looks a bit sexier (slideToggle('fast'))

I see an improvement for #30. Please take into account margins of toggle button.
Attached screenshot.

So I'd love to see #30 in core. But I think we need some more testing on devices. I tested it on WinPhone7 only.

djroshi’s picture

FileSize
5.22 KB

Thank you for the feedback :)

I think with the right-hand margin, my browser scroll-bar was getting in the way a bit. I have set it back to 10px as per the original design.

I have also wrapped a couple of hard-coded strings in t() functions.

Konstantin Komelin’s picture

Used SimplyTest.me to test #33.
The toggle button is not visible. But I might be wrong.

mjohnq3’s picture

Status: Needs review » Needs work

I tested #33 on SimplyTest.me also. The menu button doesn't appear.

djroshi’s picture

FileSize
5.12 KB

#33 generates a corrupt toggle.png file - hence the disappearing menu button. No idea how that happened. Hopefully this one works...

djroshi’s picture

I have tested #36 on simplytest.me and the menu button is back. Yay!

mjohnq3’s picture

Status: Needs work » Needs review

Looks good.

Konstantin Komelin’s picture

#36 doesn't support RTL languages.

djroshi’s picture

FileSize
6.16 KB

RTL breaks the standard bartik responsive menu too by the way :)

We actually have a bit of overlap happening here:

http://drupal.org/node/1804446

What I have done is pulled a section of that patch (the main-menu stuff) into #40 (and modified it slightly) so that this issue can address the remaining issues with the bartik responsive main menu.

I hope this is OK?

Konstantin Komelin’s picture

#40 works well for me.

mjohnq3’s picture

Patch #40 looks good to me too.

carwin’s picture

Status: Needs review » Reviewed & tested by the community

Tested, patch applies cleanly with one insignificant whitespace error. Patch works as designed and solves the problem.

Marking RTBC.

jibran’s picture

I tested it works great.
But I think we need e.preventDefault(); or javascript:void(0);

+++ b/core/themes/bartik/template.phpundefined
@@ -75,6 +75,15 @@ function bartik_process_page(&$variables) {
+  // Add a hide menu item to the main menu
+  if ($variables['main_menu']) {
+    $variables['main_menu']['menu-hide'] = array(
+      'title' => t('Hide menu'),
+      'href' => '',
+      'fragment' => ' ',
+      'external' => TRUE,
+    );
djroshi’s picture

But I think we need e.preventDefault(); or javascript:void(0);

Why?

jibran’s picture

Because it will stop browser from bubbling up when menu icon is clicked.

djroshi’s picture

There is no javascript in #40 - these click events need to be handled by the browser.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Sounds like this needs more discussion?

djroshi’s picture

Patch #40 uses CSS to toggle the menu. It does so by using an anchor href and the CSS3 :target selector. The menu opens by navigating the browser to "#main-menu-links" and closes by navigating to "#".

Of course "#" is commonly misappropriated as a href for elements that are solely used as javascript triggers. In these cases javascript:void(0) and e.preventDefault() is used to stop events being handled by the browser.

I would appreciate an explanation from jibran as to how exactly his comments are at all relevant to the patch provided.

jibran’s picture

Issue tags: +Needs usability review

Sorry for not being clear. Let me explain myself.

  • After applying the patch.
    Only local images are allowed.
  • After clicking the menu icon
    Only local images are allowed.
    see site logo is not visible only menu items are showing at the top.
  • If we'll usee.preventDefault(); or javascript:void(0); it would look like this
    Only local images are allowed.
    browser will scroll down and site logo is still visible.

So all I am saying is that page have to remain at its original place and it should slide down to accommodate menu items.
And IMHO menu items should appear under menu icon not the other way around. So adding the usability tag for more opinions.

djroshi’s picture

Please read #32 for a brief comparison of the CSS and JS approaches, and review #20 if you are interested in a javascript implementation.

The CSS approach is obviously limited by cross-browser issues. The menu items all have fluid height which is difficult to animate in CSS and the use of the :target selector results in a page jump to the named div. That said it's very, very lightweight, doesn't require javascript to be enabled, and is designed to degrade nicely if browser doesn't support the CSS :target selector.

Also, I have to say. You click the menu button - it jumps to the menu. OMG.

I think I have already explained why e.preventDefault() and javascript:void(0) are not applicable solutions. If you are not convinced then I guess you will have to try it, and see for yourself that it completely breaks the functionality. If you are going to tell the browser to not respond to click events, you better have a script or something that does - or else guess what? Nothing happens.

As for menu items above/below, I'm not really bothered either way.

To sum up, #40 has been tested as working by four people besides myself. It has two known "issues", the page jump due to named anchors, and the lack of a slide animation. Both these issues directly reflect the trade-off between the CSS and JS approaches.

IMO Bartik should be lightweight and functional. Loading jquery on every page just to enable slide animation for a toggle menu that only appears on mobile devices just doesn't seem right to me...

jibran’s picture

FileSize
518 bytes
6.18 KB

Ok let's try

Konstantin Komelin’s picture

#52 Collapsing doesn't work at least in FireFox and Chrome. Q.E.D.

djroshi’s picture

FileSize
1.94 KB
6.52 KB

This should maybe fix the page jump issue. However cross-browser compatibility now depends on both :target and adjacent sibling ~ CSS selectors being supported. I would be interested to see how well supported this is on mobile platforms. Tested in Firefox, Chrome, Safari, and Opera.

mjohnq3’s picture

FileSize
24.03 KB

Tests fine using a sandbox on simplytest.me on Chrome 25, Firefox 19, and Opera 12.5. It also works correctly with IE9 and IE10 but there's a slight visual anomaly, (stray text appears below the word Menu when its opened) as shown in the attached screenshot.

djroshi’s picture

FileSize
354 bytes
6.5 KB

Hmm, I was using a slightly newer version of the -999em hack to hide that text. Maybe the original will work better? Sorry I can't test in IE at the moment :(

Konstantin Komelin’s picture

Tested #54.
Desktop:

  • FireFox, Chrome, Opera, Safari - fine
  • IE9 - fine, but with text-indent effect described in #55

Mobile:

  • IE9 on WindowsPhone7 - the same effect #55 and short delay between click (touch) and reaction. I guess the delay depends on concrete device.
jibran’s picture

It is working fine in IE9, IE10, Chrome 25, Firefox 19. Thanks for the changes.

Konstantin Komelin’s picture

#56 works well in the latest desktop browsers (Windows) and IE9 on WinPhone7.

jibran’s picture

Issue tags: +Usability

Adding tag.

mjohnq3’s picture

#56 works correctly in IE9 and IE10.

nod_’s picture

ry5n’s picture

So nod_ pointed me here. I’ve done a redesign of the way our navigation tabs (e.g. at the top of the overlay) wok on small screens. Very similar to the standard approach to responsive navs, and to the interaction pattern applied here. However, my solution uses JS to detect available space and switch layout accordingly. Uses CSS animations for performance on small screens.

A demo is available here: http://drupalcode.org/sandbox/ry5n/1932040.git/blob_plain/ec02de3c6a05ef...

I wonder if we could decide on a best implementation for core? A basic solution that if needed, one or both (navs/tabs) can extend?

Konstantin Komelin’s picture

@ry5n, we have at least two working solutions in this issue:
1) #1 jQuery.resize is used to check size. Works everywhere, supports landscape -> portrait switching.
2) Excelent solution without Javascript dependency #56. Works. Tested.

You suggested 3d. Let's compare them.

Konstantin Komelin’s picture

FileSize
1.19 KB

Shortened url of @ry5n demo: http://goo.gl/ZLa5f
Should be better now:
chart.png

Konstantin Komelin’s picture

#63 doesn't look very responsive on Windows Phone 7. I guess due to lack of meta:viewport tag

Konstantin Komelin’s picture

Btw, now it's possible to generate QR code for simplytest.me sandbox url by clicking special icon on the timer panel.

ry5n’s picture

I’ve added a viewport meta tag and fixed some JS bugs in my implementation. Demo of the new commit is at http://bit.ly/YLcw6U

To compare all three approaches, I’m only looking at code and the post in this issue; it looks like both #20 (the most recent JS-based patch) and #56 need re-rolls.

#56. CSS-only approach: no JS needed (nice). Some of the CSS selectors look more complex than they need to be, but that doesn’t detract from the approach. Downsides mentioned in previous comments are the lack of an open/close animation and the jump caused by scrolling the page to the named anchor. I would have to try a demo to see if the experience was acceptable. Sometimes named-anchor jumps are disorienting.

#20. Basic JS approach. Obviously requires JS. Has a nicer animation for open/close (although animating in JS on mobile can perform poorly, i.e. low FPS).

An additional downside of both approaches is they depend on specific media queries to toggle layout, so both would need manual tweaking if and when the menu items change.

#67. This one :)
Advantages: falls back to a stacked nav when no js. Touch targets larger on touch screens. Layout switch happens automatically based on the actual rendered size of the element (no media queries to manage). Has a nice slide animation; uses hardware-accelerated CSS transitions for smooth performance on CPU-constrained devices (which is where it would be seen).

Disadvantages: complex. Depends on jQuery, Modernizr (touch, csstransitions). Uses several custom jQuery plugins (they are modular though, none is particularly complicated). Does use a window.resize handler. In reality though, if we assume folks won’t resize their window, the performance cost of having the resize handler is still zero :) And if the window does resize, it doesn’t break.

For me, it comes down to a couple of things. Drupal’s admin UI (Seven) needs to provide a really good experience. That means speed, but also avoiding aesthetic and usability issues (potentially) with the CSS approach. Again, I’d need to demo it to see. If we want to go with a single approach for responsive navs in Core, it should be one that can support all core themes.

So: should we build a common responsive nav implementation? If so, my tentative vote is for JS with auto-layout.

Konstantin Komelin’s picture

> if we assume folks won’t resize their window, the performance cost of having the resize handler is still zero :)
This is actual for #1 as well. I mentioned it instead of #20 because #20 doesn't use resize and therefore doesn't support landscape <-> portrait switching.

If you wanna see how the similar approach (#1) works take a look at Responsive Bartik theme demo http://drupal.org/project/responsive_bartik

I personally prefer #56. The named-anchor jump is not disorienting users because the anchors are close to each other.

Let's see what other guys think.

ry5n’s picture

@konstantin.komelin

This is actual for #1 as well.

Yes absolutely. My argument about resize handlers applies generally.

I looked at responsive_bartik, but it doesn’t seem to include the changes proposed here. I also tried http://d8.komelin.com/ but I don’t see the expected behavior. Do I need to log in?

I’d like to try both of these out, either a demo or reroll. Can someone oblige?

Konstantin Komelin’s picture

http://d8.komelin.com/ is not up to date.

Reroll is probably ideal variant because we will be able to use simplytest.me for applying patches and seeing results in action.

Konstantin Komelin’s picture

Written message to @djroshi about #56

djroshi’s picture

FileSize
6.5 KB

Reroll of #56

Status: Needs review » Needs work

The last submitted patch, bartik-1880488-73.patch, failed testing.

Konstantin Komelin’s picture

Status: Needs work » Needs review

Thanks djroshi! #73 looks good.

djroshi’s picture

#73: bartik-1880488-73.patch queued for re-testing.

ry5n’s picture

Thanks for the reroll! I still think we need JS rather than hard-coded breakpoints to switch the menu layout, for flexibility reasons. But now that I’ve tested this, I feel better about the CSS-only solution for the toggle action. It works well; the only disconcerting thing is that the menu appears above the trigger, rather than below. I feel like the place where my attention was focused shouldn’t jump away, and I should be able to tap the same spot to reverse the action.

I also realized that if we were to use the CSS-only approach as a general solution for core it would need to work inside the Overlay (because of the nav tabs). But the Overlay uses special URLs like /#overlay=admin/content, which is probably an issue since there’s already a fragment in there.

nod_’s picture

Overlay doesn't work well (at all?) on touch so we just get rid of it when that's the case (I think some code needs to be updated for that to work well but that's the idea).

I wouldn't worry too much about responsive things inside the overlay. There shouldn't be any.

ry5n’s picture

That’s true. In an ideal world it’s not only about touch or screen size, since we could still have a set of tabs that don’t fit across the overlay (especially with the toolbar in vertical mode), but I guess if it works enough of the time, it’s OK.

djroshi’s picture

Can we get get a consensus on whether the menu should appear above (as per konstantins original design) or below the menu button?

Konstantin Komelin’s picture

Much time has passed. I'm giving you guys opportunity to decide.

jibran’s picture

#73: bartik-1880488-73.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs usability review, +Usability, +mobile

The last submitted patch, bartik-1880488-73.patch, failed testing.

rteijeiro’s picture

Assigned: Unassigned » rteijeiro

Re-rolling...

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
5.44 KB

Patch re-rolled.

+++ b/core/themes/bartik/templates/page.tpl.php
@@ -144,6 +144,8 @@
+        <div id="nav"></div>
+        <div id="no-nav"></div>

Have talked with LewisNyman and maybe we should refactor this empty divs.

djroshi’s picture

Sure. What do you suggest?

LewisNyman’s picture

Status: Needs review » Needs work

Having read through the issue now, I think the CSS only approach is a decent improvement, the JS based tabs solution is still being implemented in Seven and maybe at a later date we could look at solving it in the same way. I don't think it's a major problem here though.

Let's fix the final UX issue and call it done.

Can we get get a consensus on whether the menu should appear above (as per konstantins original design) or below the menu button?

Above.

djroshi’s picture

OK I will provide a patch with the menu below the button so we can do a proper comparison...

rteijeiro’s picture

Assigned: rteijeiro » Unassigned
emma.maria’s picture

Patch #85 applies but the menu no longer collapses when you try to close it whilst on the same page no matter where you click and also still shows as open on page refresh. It only collapses again when you navigate to another page. Seems to definitely need more work.

Konstantin Komelin’s picture

Assigned: Unassigned » Konstantin Komelin
Issue tags: +drupalcampfi

Hey! I will be happy to continue working on it.

-- Konstantin

Konstantin Komelin’s picture

Assigned: Konstantin Komelin » Unassigned
Status: Needs work » Needs review
FileSize
5.71 KB
766 bytes

Improved #85

Please review.

Thanks,
Konstantin

ohthehugemanatee’s picture

patch applies cleanly, and the menu is collapsible.

But the toggle.png icon does not appear on my dev environment, because the url generated for it doesn't include the port number (I work on http://localhost:8080/). In css/style.css:618

body:not(:target) #main-menu-reveal:after {
  content:"";
  background: url('../images/toggle.png') no-repeat;
  width: 22px;
  height: 22px;
  display: inline-block;
  position: absolute;
  right: 10px; /* LTR */
}

This produces "background: url('http://localhost/core/themes/bartik/images/toggle.png') no-repeat scroll 0% 0% transparent;" . That URL doesn't get me anywhere. :(

ohthehugemanatee’s picture

Status: Needs review » Needs work
sqndr’s picture

+++ b/core/themes/bartik/css/style.css
@@ -594,6 +595,64 @@ h1.site-name {
+  background: url('../images/toggle.png') no-repeat;

@@ -1883,6 +1951,9 @@ div.admin-panel .description {
diff --git a/core/themes/bartik/images/toggle.png b/core/themes/bartik/images/toggle.png

+++ b/core/themes/bartik/images/toggle.png
@@ -0,0 +1,4 @@
IHDRĴl;tEXtSoftwareAdobe ImageReadyq�e<"iTXtXML:com.adobe.xmp<?xpacket begin="" id="W5M0MpCehiHzreSzNTczkc9d"?> <x:xmpmeta xmlns:x="adobe:ns:meta/" x:xmptk="Adobe XMP Core 5.3-c011 66.145661, 2012/02/06-14:56:27        "> <rdf:RDF xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"> <rdf:Description rdf:about="" xmlns:xmp="http://ns.adobe.com/xap/1.0/" xmlns:xmpMM="http://ns.adobe.com/xap/1.0/mm/" xmlns:stRef="http://ns.adobe.com/xap/1.0/sType/ResourceRef#" xmp:CreatorTool="Adobe Photoshop CS6 (Windows)" xmpMM:InstanceID="xmp.iid:36A1E4F94D2711E285EEEC7EA196AFD7" xmpMM:DocumentID="xmp.did:36A1E4FA4D2711E285EEEC7EA196AFD7"> <xmpMM:DerivedFrom stRef:instanceID="xmp.iid:36A1E4F74D2711E285EEEC7EA196AFD7" stRef:documentID="xmp.did:36A1E4F84D2711E285EEEC7EA196AFD7"/> </rdf:Description> </rdf:RDF> </x:xmpmeta> <?xpacket end="r"?>"E���IDATx�ԕ;�0D�"%����\)}�tA

Why are we using a png image instead of an svg? Anyway ... seems like there's lot's of *.png's in Bartik.

If you're adding a new image: you should also get you're image optimised using something like ImageOptim. http://cl.ly/image/1f0f0v3k1e1u

sqndr’s picture

Yeah. So - there already is an svg image (hamburger.svg) in core in the toolbar. Why aren't we reusing that one? It's *.svg, so it's retina-ready and it's already in the core http://cl.ly/image/1k3p2h3X0a0J

Konstantin Komelin’s picture

Assigned: Unassigned » Konstantin Komelin

@ohthehugemanatee thank you for review!
I've tested the patch on 8080 port, it's working fine. It can be a local cache issue.
Anyway, the PNG will be replaced soon (see below).

@sqndr,
I will try to recreate the patch with the hamburger.svg. Thx for the suggestion.

-- Konstantin

sqndr’s picture

body:not(:target) #main-menu-reveal:after {
  content:"";
  background: url('../../../misc/icons/bebebe/hamburger.svg') no-repeat;
  width: 22px;
  height: 22px;
  display: inline-block;
  position: absolute;
  right: 10px; /* LTR */
}

Something like this.

Konstantin Komelin’s picture

Assigned: Konstantin Komelin » Unassigned
Status: Needs work » Needs review
FileSize
4.22 KB
1.8 KB

Improved #92.
Replaced PNG with existing SVG.

Please review.

Thanks,
Konstantin

sqndr’s picture

+++ b/core/themes/bartik/css/style.css
@@ -594,6 +595,65 @@ h1.site-name {
+  background: url('../../../misc/icons/ffffff/hamburger.svg') no-repeat;

Yeah! This looks good to me. Nice job!

Konstantin Komelin’s picture

@sqndr,
Thanks! Can we set the issue to RTBC?

rteijeiro’s picture

Status: Needs review » Needs work
FileSize
30.23 KB
41.46 KB
+++ b/core/themes/bartik/templates/page.html.twig
@@ -121,7 +121,10 @@
+        <div id="nav"></div>
+        <div id="no-nav"></div>
         {{ main_menu }}
+        <a id="main-menu-reveal" href="#nav">{{ 'Menu'|t }}</a>

I think we should use classes instead ids.

Anyway everything looks fine. Attached a couple of screenshots.

Konstantin Komelin’s picture

Thanks @rteijeiro for the review. I'll replace IDs with classes.

rteijeiro’s picture

Status: Needs work » Reviewed & tested by the community

After discussing this with konstantin.komelin we found that ids are required for this solution.

Then it's a RTBC for me.

Konstantin Komelin’s picture

Great! Thanks to everyone who was involved!

alexpott’s picture

This looks great - however I'm not 100% convinced by the menu appearing above the toggle - that seems counter-intuitive to me since the thing you've just clicked is always going to move and to close it if you didn't mean to open the menu you're going to have to scroll.

Bojhan’s picture

Assigned: Unassigned » LewisNyman

I man not sure I understand that part either. Most menu buttons are persistent. The reason we place it above on Seven is because the visual design doesn't really allow for it to be placed below. For Bartik I am not sure if we need to do this.

@Lewis could you elaborate more?

Bojhan’s picture

Status: Reviewed & tested by the community » Needs review
LewisNyman’s picture

Issue summary: View changes

After reading my comment, I must of confused myself. One word answers must not be my strong point...

We definitely do want the button to stay in the same place. I've updated the summary to include this remaining task.

Amad Tababa’s picture

Issue summary: View changes
FileSize
236.5 KB
Konstantin Komelin’s picture

Issue summary: View changes

Removed irrelevant information from the issue description.

Amad Tababa’s picture

FileSize
216.92 KB

The batch ( bartik-made-menu-collapsible-1880488-98.patch ) I have tested this patch and seems work fine for me with different device. this is a screen example with emulator S4.

Amad Tababa’s picture

Status: Needs review » Reviewed & tested by the community
Amad Tababa’s picture

FileSize
216.92 KB
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

After discussion with me and @Bojhan in #109 @LewisNyman says

We definitely do want the button to stay in the same place. I've updated the summary to include this remaining task.

So this can not be rtbc.

Konstantin Komelin’s picture

Thanks guys. Unfortunately, it was designed to have the toggle button at the bottom, so I can't help to remake it.

@Amad Tababa, your device emulator is not a real device emulator. It can behave differently in some cases. I recommend either testing on real devices and official emulators of manufacturers or at least using BrowserStack.

LewisNyman’s picture

This is my fault in a way. I'll sort it out.

LewisNyman’s picture

Status: Needs work » Needs review
Issue tags: -Needs usability review
FileSize
2.77 KB
4.02 KB
67.53 KB

Woo.

LewisNyman’s picture

Issue summary: View changes
LewisNyman’s picture

Sorry! I forgot to test desktop. Please review this patch.

ekl1773’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me- tested with emulators, menu collapses appropriately, code looks good to me.

Bojhan’s picture

Ok, lets get this in!

nod_’s picture

No JS hey, we should have more patches like that :D

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c20145f and pushed to 8.x. Thanks!

  • Commit c20145f on 8.x by alexpott:
    Issue #1880488 by djroshi, konstantin.komelin, LewisNyman, jibran,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

iMiksu’s picture

Issue tags: -drupalcampfi

Cleaning up drupalcampfi tags.