New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When setting $CFG->additionalhtmlfooter, the final HTML output contains a duplication of it #9

Closed
scara opened this Issue Nov 21, 2015 · 10 comments

Comments

Projects
None yet
2 participants
@scara
Contributor

scara commented Nov 21, 2015

That is because if you want to put $CFG->additionalhtmlfooter in another place you should also override standard_end_of_body_html() to not include it, since you've already added it in your standard_footer_html().

I could create a PR but I'm not sure what should be the right fix on behalf of you:

  1. override standard_end_of_body_html();
  2. remove the $CFG->additionalhtmlfooter related code and let the base standard_end_of_body_html() do its work.

My vote goes to (b) since you already have theme_morecandy | footnote for a theme related footnote.

@lazydaisy

This comment has been minimized.

Show comment
Hide comment
@lazydaisy

lazydaisy Nov 21, 2015

Owner

Hi, I am not quite sure what you mean? As far as I know I have not added an additional footer to the layout in Moecandy theme. Or is this issue on behalf of someone else who is using this theme?

Owner

lazydaisy commented Nov 21, 2015

Hi, I am not quite sure what you mean? As far as I know I have not added an additional footer to the layout in Moecandy theme. Or is this issue on behalf of someone else who is using this theme?

@lazydaisy

This comment has been minimized.

Show comment
Hide comment
@lazydaisy

lazydaisy Nov 21, 2015

Owner

As you can see here I am using Clean themes footer code.
https://github.com/lazydaisy/moodle/blob/master/theme/clean/layout/columns3.php#L96-L105
So if this is wrong then Clean theme lib.php is wrong because that is where the footnote is coded.
Sam Hemalric did all those changes, last year.

Owner

lazydaisy commented Nov 21, 2015

As you can see here I am using Clean themes footer code.
https://github.com/lazydaisy/moodle/blob/master/theme/clean/layout/columns3.php#L96-L105
So if this is wrong then Clean theme lib.php is wrong because that is where the footnote is coded.
Sam Hemalric did all those changes, last year.

@scara

This comment has been minimized.

Show comment
Hide comment
@scara

scara Nov 21, 2015

Contributor

I mean:

if (!empty($CFG->additionalhtmlfooter)) {

Steps to reproduce:

  1. Install Morecandy theme: do not perform any configuration just accept the defaults
  2. Select this theme in place of the default one, Clean
  3. Go to Site administration > Appearance > Additional HTML and put <p>Test</p> in additionalhtmlfooter
  4. Look at the footer: the text is doubled.

That doesn't happen if you choose to add a footnote via Site administration > Appearance > Themes > Morecandy, using theme_morecandy | footnotev.

The goal of the two settings are IMHO quite different, the first being more oriented to injecting something like tracking code, the latter for a readble footnote.

HTH,
Matteo

Contributor

scara commented Nov 21, 2015

I mean:

if (!empty($CFG->additionalhtmlfooter)) {

Steps to reproduce:

  1. Install Morecandy theme: do not perform any configuration just accept the defaults
  2. Select this theme in place of the default one, Clean
  3. Go to Site administration > Appearance > Additional HTML and put <p>Test</p> in additionalhtmlfooter
  4. Look at the footer: the text is doubled.

That doesn't happen if you choose to add a footnote via Site administration > Appearance > Themes > Morecandy, using theme_morecandy | footnotev.

The goal of the two settings are IMHO quite different, the first being more oriented to injecting something like tracking code, the latter for a readble footnote.

HTH,
Matteo

@lazydaisy

This comment has been minimized.

Show comment
Hide comment
@lazydaisy

lazydaisy Nov 21, 2015

Owner

Ah...now I see what you mean!
Ok...the only reason that renderer function is overriding is related to the link for 'Purge all caches' and the three links for Validation. All these links are now buttons with an icon.
So the fact that additional footer is there is because I was not aware of what it did, in fact I can honestly say I did not know it was there, it is just part of the renderer.
Perhaps there is a better way to change these links to convert them to buttons with font icons...rather than using a renderer.

Owner

lazydaisy commented Nov 21, 2015

Ah...now I see what you mean!
Ok...the only reason that renderer function is overriding is related to the link for 'Purge all caches' and the three links for Validation. All these links are now buttons with an icon.
So the fact that additional footer is there is because I was not aware of what it did, in fact I can honestly say I did not know it was there, it is just part of the renderer.
Perhaps there is a better way to change these links to convert them to buttons with font icons...rather than using a renderer.

@scara

This comment has been minimized.

Show comment
Hide comment
@scara

scara Nov 21, 2015

Contributor

Eh eh... that could be a nice exercise to be posted into the Moodle Dev Community: "copying" the renderer means to keep it aligned with the master source while here it could be better to take the output of the base renderer and change it using RegExp to extract the links and turn them into "buttons", if the renderer changes frequently.
My suggestion for the quick fix is: align your copy with the last changes.

Refs, when the issue came out:

How I did the search? $ git log -G"$CFG->additionalhtmlfooter" -- lib/outputrenderers.php

Contributor

scara commented Nov 21, 2015

Eh eh... that could be a nice exercise to be posted into the Moodle Dev Community: "copying" the renderer means to keep it aligned with the master source while here it could be better to take the output of the base renderer and change it using RegExp to extract the links and turn them into "buttons", if the renderer changes frequently.
My suggestion for the quick fix is: align your copy with the last changes.

Refs, when the issue came out:

How I did the search? $ git log -G"$CFG->additionalhtmlfooter" -- lib/outputrenderers.php

@lazydaisy

This comment has been minimized.

Show comment
Hide comment
@lazydaisy

lazydaisy Nov 21, 2015

Owner

Gosh...I remember that Tim changed the outputrenderer...strange how these things come back to haunt you!
The method you said to use to remove the links and convert them to buttons...would that be difficult to do?

Owner

lazydaisy commented Nov 21, 2015

Gosh...I remember that Tim changed the outputrenderer...strange how these things come back to haunt you!
The method you said to use to remove the links and convert them to buttons...would that be difficult to do?

@scara

This comment has been minimized.

Show comment
Hide comment
@scara

scara Nov 21, 2015

Contributor

Here is my PRoposal: scara@d1054e9.

HTH,
Matteo

Contributor

scara commented Nov 21, 2015

Here is my PRoposal: scara@d1054e9.

HTH,
Matteo

@lazydaisy

This comment has been minimized.

Show comment
Hide comment
@lazydaisy

lazydaisy Nov 22, 2015

Owner

My goodness! I did not know you can do things like that...that's brilliant! And so neat!
Yes that does help a lot. Can I use this?

Owner

lazydaisy commented Nov 22, 2015

My goodness! I did not know you can do things like that...that's brilliant! And so neat!
Yes that does help a lot. Can I use this?

@scara

This comment has been minimized.

Show comment
Hide comment
@scara

scara Nov 22, 2015

Contributor

That's the power of OOP 😉: if you need to/can enrich the "base" output, just do it.

  • What is the drawback? A small (but still present) performance hit given by the regexp replacements
  • What is the pro? You don't need to keep your renderer in sync with its "parents" and if something will change in the future, the worst thing that could happen is a replacement missing in case of changes that will never match i.e. not bugs already fixed in core (security ones included) in case of a renderer alignment missing.

Yes you can use it if you like it, including the pros&cons described above.
You can merge it from GitHub or via plain CLI using e.g. the following lines of commands:

$ cd <your git local repo folder>
$ git checkout <branch_name>
$ git fetch https://github.com/scara/moodle-theme_morecandy Issue_9_CFG-additionalhtmlfooter_Duplication
$ git cherry-pick FETCH_HEAD

This will add my commit on top of yours and it will still credit that change to me (if you want to preserve it).
Please test it in the branches you'll apply the PRoposal on: I tested just 3.0 on master.

HTH,
Matteo

Contributor

scara commented Nov 22, 2015

That's the power of OOP 😉: if you need to/can enrich the "base" output, just do it.

  • What is the drawback? A small (but still present) performance hit given by the regexp replacements
  • What is the pro? You don't need to keep your renderer in sync with its "parents" and if something will change in the future, the worst thing that could happen is a replacement missing in case of changes that will never match i.e. not bugs already fixed in core (security ones included) in case of a renderer alignment missing.

Yes you can use it if you like it, including the pros&cons described above.
You can merge it from GitHub or via plain CLI using e.g. the following lines of commands:

$ cd <your git local repo folder>
$ git checkout <branch_name>
$ git fetch https://github.com/scara/moodle-theme_morecandy Issue_9_CFG-additionalhtmlfooter_Duplication
$ git cherry-pick FETCH_HEAD

This will add my commit on top of yours and it will still credit that change to me (if you want to preserve it).
Please test it in the branches you'll apply the PRoposal on: I tested just 3.0 on master.

HTH,
Matteo

@lazydaisy

This comment has been minimized.

Show comment
Hide comment
@lazydaisy

lazydaisy Nov 22, 2015

Owner

Thank you Matteo,
I have done just that and so thrilled it went smoothly!
Thank you very much for reporting this and helping me fix it in this way.
Mary

Owner

lazydaisy commented Nov 22, 2015

Thank you Matteo,
I have done just that and so thrilled it went smoothly!
Thank you very much for reporting this and helping me fix it in this way.
Mary

@lazydaisy lazydaisy closed this Nov 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment