search

moodle-an-hochschulen / moodle-local_boostnavigation

Moodle plugin which tries to overcome some fixed appearance behaviours of Boost's nav drawer in a clean way
GNU General Public License v3.0
39 stars 28 forks source link

Collapse "My Courses" greys out course link in breadcrumb #36

Closed jonlan closed 4 years ago

jonlan commented 5 years ago

Hello We found this Issue

To reproduce:

see Attachment "mycourses.pdf"

jonlan commented 5 years ago

mycourses.pdf

abias commented 5 years ago

Hi Jonas,

wow, I didn't spot this phenomenon before. But a case like this is why this plugin is called "Boost navigation fumbling" :/

The root of this phenomenon is lying here: https://github.com/moodle/moodle/blob/master/lib/outputrenderers.php#L3468-L3470

local_boostnavigation somehow abuses the "hidden" attribute of a navigation node for realizing its collapse feature as it's the best attribute available and as plugins can't add their own attributes to navigation nodes.

But if a navigation node is marked as "hidden", the breadcrumb node is getting the "dimmed_text" CSS class and thus is shown in grey.

We could try to introduce some other fumbling hack which solves this phenomenon. But I have the strong feeling that we should better try to rewrite the current implementation of collapsing the nodes to get rid of this phenomenon without introducing new side-effects.

We will have a look at this for the upcoming 3.7 version of the plugin. Please stay tuned.

Cheers, Alex

christianwolters commented 4 years ago

Hi @ll,

thank you Jonas for pointing this out - we just recently startet using this plugin, I saw the dimmed text but did not make the connection to collapsing the my courses node.

Fumblingmode on:

In the method Alex has metioned, there is a test only on the hidden attribute:

if ($item->hidden) {
    $attributes['class'] = 'dimmed_text';

I had a quick look at the exapanding and collapsing functions in collapsenavdrawernodes.js:

I test this later, just wanted to doc my thoughts here.

Update: ok ok...that is not feasible, the DOM manipulation of course does not survive page reload... So we would have to store the state seperately (preference api?) and apply this when rendering the nodes?

Best, Christian

danowar2k commented 4 years ago

Well, I think I found another instance where the grey links appear...

You change stuff in the nav drawer, but really you're changing the global navigation itself. So, for example, when using a "clean" based theme (Yeah I know, but we're in the process of migrating) the plugin settings mess with layout in the Navigation block that's not present anymore in Boost.

There's an easy fix for people who want to migrate to a Boost based theme but can't do this on short notice: Check whether "boost" is a parent of the current theme and just don't apply the settings if it isn't. I'm gonna scour for the correct code and maybe do a pull request for that.

I mean, the plugin is called "boostnavigation", so preventing it to work with anything other than a boost based theme should be logical, shouldn't it?

EDIT: If you don't want to shut people out who aren't using a boost based theme, just add a setting "Experimental Mode" or "Allow changes to be applied to non-boost based themes" or something with a default set to "off".

danowar2k commented 4 years ago

Basically like this (without the "experimental mode"):

function local_boostnavigation_extend_navigation(global_navigation $navigation) {
    global $CFG, $PAGE, $COURSE;

    $isBoostBasedTheme = $PAGE->theme->name == "boost" || in_array("boost", $PAGE->theme->parents);
    if (!$isBoostBasedTheme) return;
abias commented 4 years ago

I am happy to report today that this issue has finally been resolved in https://github.com/moodleuulm/moodle-local_boostnavigation/commit/f3868412c0e65b63d419882b66b45b10cebfa46b during the work on #49 in a clean and future-proof way. All abuse of existing data-* attributes in Moodle core has been removed. An official 3.8 version of this plugin will be pushed within the next few days to the Moodle plugins repo as well.

Cheers, Alex

PS: @danowar2k - I think your addition is legitimate, but this is not the issue to deal with it. Reading your post for the first time, I would have just responded: Well, this plugin is targeted at Boost. If you don't use Boost, don't install this plugin. But you are right: Some features of this plugin modify the global navigation itself irrespective of the theme which is shipped to the user. And there may be sites which are using Boost and Non-Boost themes at the same time. So, thumbs up for implementing a kill switch like you have proposed it. I would be grateful if you could open a dedicated issue and submit a pull request.