gjb2048 / moodle-theme_essential

The Essential Moodle Theme
https://gjb2048.github.io/moodle-theme_essential/
GNU General Public License v3.0
91 stars 120 forks source link

Essential Custom Menu Hamburger Behaviour. #530

Closed sam-bg closed 9 years ago

sam-bg commented 9 years ago

Hi,

In the latest 2.8.1.8 the collapsed custom menu doesn't seem to behave as it should and displays like this: 902 The menu items are hidden and a scroll bar is displayed to the right.

I've tested the previous 2.8.1.7 release and the expected behaviour is there: 901

Changing ..style/essential.css: .navbar-inner .nav-collapse.collapse.in.shown {overflow-y: visible;} to: .navbar-inner .nav-collapse.collapse.in.shown {overflow: visible;}

Seems to fix the problem. Tested on IE11, FF and chrome. Moodle 2.8.

Cheers, Sam.

gjb2048 commented 9 years ago

Hi Sam,

{overflow: visible;} as implemented in: https://github.com/gjb2048/moodle-theme_essential/commit/925ef042c105336b9f982abf8e21c432e25962b8 and https://github.com/gjb2048/moodle-theme_essential/commit/14b2d38b4ad11c040117961fe1333a66d27805b1 breaks the custom jQuery implemented to cope with issue #517 then refixed in https://github.com/gjb2048/moodle-theme_essential/commit/ed73cc64c3e6371a087a0ed5b5b9fc86488f4529#diff-e36bfc98d01ee53444b4e4a5d38dc151 then fixed again in issue #528. Thus 'overflow-y' worked for me in testing.

Painful eh!

Cheers,

Gareth

gjb2048 commented 9 years ago

Have you Purged all caches and cleared the local browser cache?

gjb2048 commented 9 years ago

Ok, I can replicate in FireFox :( - I love this theme - not.

gjb2048 commented 9 years ago

Still need to fix in M2.9.

sam-bg commented 9 years ago

It is painful. Have you considered doing something different? Painting or social work for instance? ;p Yes to the caches. It's been painful just tracking doen the problem. As I'm hacking our own custom theme I started from a cloned and renamed version and the 'overflow-y' edit doesn't work in my version but does in essesntial so I've had to have clean installs of the latest and previous versions in test environments to track it down. now I have to figure out how to fix it in our theme.

Well done on the theme and my sympathys for your workload.

gjb2048 commented 9 years ago

Ok, worked out a CSS only solution and stripped out JS (https://github.com/gjb2048/moodle-theme_essential/commit/0113b4c64182fbc3f4c71863874ee11c4f0c7afd), however when using 'overflow: visible' the JS still crashes!!! Seems to be a core bug:

2015-08-25 14_54_59-moo29_ administration_ notifications 2015-08-25 14_55_51-moo29_ administration_ notifications

gjb2048 commented 9 years ago

Commit with the 'visible': https://github.com/gjb2048/moodle-theme_essential/commit/47fdfb5175c6fd4f8b409958e37a24c3bedf989f - now broken because of:

2015-08-25 14_59_49-moo29_ administration_ notifications

gjb2048 commented 9 years ago

Broken in Clean but does not affect things:

2015-08-25 15_02_50-moodle 29

gjb2048 commented 9 years ago

https://tracker.moodle.org/browse/MDL-51201 raised.

gjb2048 commented 9 years ago

https://moodle.org/mod/forum/discuss.php?d=318870 also posted.

gjb2048 commented 9 years ago

Ok, until MDL-51201 is understood / rectified then the only workaround I can currently think of is a 'min-height' one: https://github.com/gjb2048/moodle-theme_essential/commit/cd201032d5ac0e6f5586c93867e63448fc726c43.

lazydaisy commented 9 years ago

Is this something to do with jQuery and YUI changeover in 2.9? If you recall the divider in menu did not work, because of changes made by Jetha Chan in responsive.less where active was charged it 'in' I am not too sure why! But could be the cause if you have older css in Essential? Just a thought! Mary

gjb2048 commented 9 years ago

Hi Mary,

Thank you for your thoughts. I think I've changed the CSS to be 'in' but it does appear to be an issue / conflict with jQuery / YUI in core that's killing the JS on that element, and that in the tracker issue I've proven that the bug exists on the console even when Essential is nowhere to be seen.

To be fair some of this is theoretical guesswork based on observation of what is going on, so I'm open to any other suggestions.

Sent from my iPod in deep space...

On 25 Aug 2015, at 19:52, Mary Evans notifications@github.com wrote:

Is this something to do with jQuery and YUI changeover in 2.9? If you recall the divider in menu did not work, because of changes made by Jetha Chan in responsive.less where active was charged it 'in' I am not too sure why! But. At be the cause if you have older css in Essential? Just a thought! Mary

— Reply to this email directly or view it on GitHub.

gjb2048 commented 9 years ago

P.s. at the moment it appears that the breakage of the JS on that element causes the code in the browser to fail when the CSS attribute overflow changes, so however it works / implemented = not good, and yet the code for min-height and background is fine. Thus I can assume that overflow changing causes an event / event watch where the thread that deals with it is either deadlocked or has died.

Sent from my iPod in deep space...

On 25 Aug 2015, at 19:52, Mary Evans notifications@github.com wrote:

Is this something to do with jQuery and YUI changeover in 2.9? If you recall the divider in menu did not work, because of changes made by Jetha Chan in responsive.less where active was charged it 'in' I am not too sure why! But. At be the cause if you have older css in Essential? Just a thought! Mary

— Reply to this email directly or view it on GitHub.

gjb2048 commented 9 years ago

Proper solution now stalled by: https://tracker.moodle.org/browse/MDL-51201.

gjb2048 commented 9 years ago

https://tracker.moodle.org/browse/MDL-51201 - now no longer has an effect. Odd things are happening in the jQuery.

gjb2048 commented 9 years ago

https://github.com/gjb2048/moodle-theme_essential/commit/3d52503cf48bb8dd73e0e837fbb2a57a827dd4ac demonstrates that the presence of the 'overflow: visible' kills the 'complete()' function call at the end of the transition event.

gjb2048 commented 9 years ago

Pulling my hair out: https://github.com/gjb2048/moodle-theme_essential/commit/256a7bef25235e14048e1195d92b4f57b4d7931f

gjb2048 commented 9 years ago

For the life of me I cannot solve this, going to have to live with the min-height workaround: https://github.com/gjb2048/moodle-theme_essential/commit/f980a41c8ee92d39bdd23f584f45f62509a11b2e

lazydaisy commented 9 years ago

You mean take up life as a Hobbit?

gjb2048 commented 9 years ago

LOL, no! Roadsign painter.

lazydaisy commented 9 years ago

Hey that's a lucrative occupation!

gjb2048 commented 9 years ago

Indeed :) - already made a start: http://gjbarnard.deviantart.com/art/Safe-height-396032609 and http://gjbarnard.deviantart.com/art/Parody-of-a-road-sign-546332650

gjb2048 commented 9 years ago

Ok,

This seems to be related to the fact that in transitioning to using the AMD method of loading the BS JS this happens.

So, to replicate, use the 'overflow' attribute in less/essential/responsive.less:

            &.collapse.in[style*="height: auto"] {
                min-height: 768px;
                //overflow: visible;  // Definitely killing the complete() function (line: 157 in bootstrap.js) being fired at the end of the transition.
            }

instead of 'min-height'. Recompile the CSS from the LESS with grunt. Reload the page. Shrink the page such that the menu has the hamburger with cheese menu. Click on the 'hamburger with cheese' icon, menu opens, click on it again, menu closes, click on it again, menu fails to open.

Somehow the changing value of 'overflow' causes the event to fail and no longer operate.

Essential needs this 'overflow' because normal Bootstrap on shrunken screens does not have content (menus) that extend beyond the collapsed area. But Essential does with the dropdown menus.

I think this is all related to MDL-51201 and MDL-51585. In addition I think that perhaps there is not the correct 'faatory' method of loading the Bootstrap JS, https://github.com/moodle/moodle/blob/MOODLE_29_STABLE/theme/bootstrapbase/amd/src/bootstrap.js#L21:

define(['jquery'], function($) {

Need to think!

gjb2048 commented 9 years ago

Try:

@media (max-width: 979px)
.nav-collapse, .nav-collapse.in.collapse {
overflow: visible;
}
gjb2048 commented 9 years ago

Hopefully fixed in: https://github.com/gjb2048/moodle-theme_essential/commit/2a4a6bfc59284c793f2808c0646dc45321bfc259