Adaptable 1.3 Causing Ticker to Consume all of Front Page Items

[Joe Downes]

I am using Moodle 3.1.3 and the Adaptable theme Ticker looks to be consuming all of the items on the front page, and cycling through them all including the marketing blocks, the news items and even going down to the footer.

This is a screenshot of the marketing blocks contained within the ticker:

Hope you can help.

[Info 3bits]

This is not a bug. Probably you added some wrong tag to the ticker.

Please, use the moodle forum for your questions and read the README.md before install and set up Adaptable.

[Nick Phillips]

It is a bug, caused by commit 29b532f.

[Nick Phillips]

See PR30 for fix - https://bitbucket.org/covuni/moodle-theme_adaptable/pull-requests/30/fix-get_news_ticker-in-renderersphp-covuni/diff

[Nick Phillips]

Oops, forgot last commit. Will update PR shortly.

[Nick Phillips]

Fixed. PR now good.

[Nick Phillips]

OK, this is definitely still broken. I'll fix it, but I'm having trouble working out what the intention is. When there are multiple ticker sections in settings, should there be:

• multiple tickers stacked down the page, each potentially visible to a subset of users?
• one ticker with each ticker section in settings used to create one ticker item, if appropriate for current user?
• one ticker with each ticker section able to contain multiple ticker items, and all applicable ticker sections for the current user combined?

I think the second of these is the only one that is actually going to work well. If it is intended that each section be able to hold multiple ticker items (i.e. child elements of the #ticker element) then the help text for the ticker sections should probably mention that each ticker item needs to be enclosed in <li> (or whatever, depending on what element is used for #ticker).

Whatever, either everything should be added within the foreach, or only the msg should be. There is no way on earth it can work correctly with the <ul> within it and the </ul> outside.

[Info 3bits]

The issue is already fixed and probably will keep only one ticker. More than one has not so much sense.

[Info 3bits]

Bug raises when more one ticker is used

[Nick Phillips]

OK, two possible fixed versions, one for option 2, one for option 3. Will create two PRs, and you can pick which you prefer, or DIY.

[Jeremy Hopkins]

Hi Nick,

The intention for multiple tickers is to be able to target them to differnet audiences.

We have two, one containing items for all users, the other targeted only at staff.

Staff then see the "all users" messages and additionally those targetted only toward staff users.

As with many other parts of adaptable you will see in the settings you will see the option to put a custom_profile_field_name=value rule into each of the tickers to facilitate this.

This should be a really easy fix, the original code just built a list of values to put into the ticker code, from memory it should have all gone into the same array, my guess is there is just a tag out of place within the loop that builds that?

[Jeremy Hopkins]

Fix #597 The previous code was adding divs inside the loop used to build multiples messages.

The $msg must be built in the loop before being inserted into the rest of the code.

As line 754 needs to call the setting directly that has to be contained within the loop also.

I think part of the problem with this previously may have been the line that used regex to strip out incorrect tags, previously the list (li) tag was being used and the regex stripped out all other tags including "p". So when the code was switched to require "p" tags this regex should have been updated to strip out li and not p.

→ <<cset 4f99ad7b2a0c>>

[Nick Phillips]

Yep, hard bit was realising how it was intended to work :-)

[Nick Phillips]

Wasn't sure whether the regex was intended to strip out tags that WYSIWYG editor might put in around empty content and was there purely to test for empty content, or whether it was actually intended to strip it out in content actually to be used.

[Jeremy Hopkins]

Yes that is exactly what the regex is for, but at some point the script was changed so that instead of needing li tags we now need p tags.

The regex as it was was still set up to strip out p tags and not li, so I think perhaps one of the issues that lead to this muddle was that the p tags were being stripped out.

Have you tested the latest commit I made to see if it works ok?

[Nick Phillips]

I'll test it tomorrow at work - from a very quick look, it seems to me like it is now stripping them from the actual content used, rather than purely to test whether it's empty.

[Jeremy Hopkins]

Minor FIx #597

→ <<cset b3d7a2dc1d55>>

[Jeremy Hopkins]

Im not sure what you mean but the regex will always strip out tags, if msg is empty a default one will display.

I just corrected a couple of things I missed yesterday.

One other point, this was only intended for short text messages, not "content" as such, for that we have the larger sliders in the theme.

[Nick Phillips]

oops. commented on wrong version :-/

What I meant before was basically just checking that your last sentence was actually the intention, as it wasn't clear the way the code had ended up.

Commenting on the right version now... seems to work ok. Only thing is, if using <p> as ticker item tag, it makes sense to use a <div> rather than a <ul> as the ticker element i.e.

<div class="ticker">
  <p>tick</p>
  <p>tock</p>
  <p>etc.</p>
</div>

The js just uses whatever the top-level elements within the .ticker element are as ticker items.

[Nick Phillips]

I'd suggest that it would be cleaner to use "continue" instead of setting $access = false and get rid of the if ($access) below, too:

            if (!empty($PAGE->theme->settings->$profilefield)) {
                $profilevals = explode('=', $PAGE->theme->settings->$profilefield);
                if (!$this->check_menu_access($profilevals[0], $profilevals[1], $textfield)) {
                    $access = false;
                }
            }

            if ($access) {
                $msg .= format_text($PAGE->theme->settings->$textfield, FORMAT_HTML, array('trusted' => true));
            }

to

            if (!empty($PAGE->theme->settings->$profilefield)) {
                $profilevals = explode('=', $PAGE->theme->settings->$profilefield);
                if (!$this->check_menu_access($profilevals[0], $profilevals[1], $textfield)) {
                    continue;
                }
            }

            $msg .= format_text($PAGE->theme->settings->$textfield, FORMAT_HTML, array('trusted' => true));

[Jeremy Hopkins]

Im not sure what changed to make it use p as opposed to li, I saw Fernando made mention of changing the script to support multi lang but am not sure if that meant the js used to render this, i could not see any js changes.

I would really want Fernando to comment before altering anything else as he made the previous changes which I dont really understand.