Skip to content
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.

Sign up for GitHub

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

Jump to bottom

web: add ability to put extra stuff in <head> element of all pages #2872

Merged
merged 2 commits into from
Jan 10, 2019
Merged

web: add ability to put extra stuff in <head> element of all pages #2872

merged 2 commits into from
Jan 10, 2019

Conversation

davidpanderson
Copy link
Contributor

project.inc can include a constant HEAD_EXTRA.
Its contents are inserted at the beginning of tags.

This makes it possible to use Google Analytics,
which requires that a script be added at that point.

Fixes #

Description of the Change

Alternate Designs

Release Notes

@davidpanderson
web: add ability to put extra stuff in <head> element of all pages
cb1aa22 
project.inc can include a constant HEAD_EXTRA.
Its contents are inserted at the beginning of <head> tags.

This makes it possible to use Google Analytics,
which requires that a script be added at that point.
@TheAspens TheAspens self-assigned this Dec 13, 2018
@TheAspens
Copy link
Member

The function page_head includes this parameter

    $head_extra=null
        // extra stuff to put in <head>. E.g.:
        // reCAPTCHA code (create_profile.php)
        // bbcode javascript (forums)

and in the code down lower there is your new code followed by the existing code

    if (defined('HEAD_EXTRA')) {
        echo HEAD_EXTRA;
    }
    echo '
        <meta name="viewport" content="width=device-width, initial-scale=1">
    ';
    if ($head_extra) {
        echo "\n$head_extra\n";
    }

based on the description of the function parameter, it exists to serve the same purpose as your new code. It seems like there is overlap between your code and this existing mechanism. Does the existing mechanism not meet your need? Do we know if anyone uses the existing mechanism? Is there some way that the two mechanisms can be combined so that there isn't any overlap?

@davidpanderson
Copy link
Contributor Author

Google analytics says their script must go at the start of the

@Rytiss
Copy link
Contributor

Rytiss commented Dec 14, 2018

The Google documentation (https://developers.google.com/analytics/devguides/collection/analyticsjs/) specifies "near the top of the tag".

I've experienced zero difference in the performance of the tracking code based on tag locations - it just works. I usually put it as the last tag in the <head>.

I believe there is no need for an additional custom head section, especially considering that the new and old sections are only separated by a single meta tag, which does not load any assets and thus does not slow down the page load.

@davidpanderson
Copy link
Contributor Author

The main thing is, this puts it in every page,
without modifying every single page_head() call

@TheAspens
Copy link
Member

Ah - so the parameter on the function is for use on a "per page" basis while the variable you have defined is more of a "global" setting.

Is it worth calling it GLOBAL_HEAD_EXTRA to clarify the difference? Let me know what you think and then I'll merge.

@TheAspens
Copy link
Member

(process note - since Rytiss voiced an concern - he needs to indicate he is ok with the solution as well before I merge)

@Rytiss
Copy link
Contributor

Rytiss commented Dec 14, 2018

I'm ok with this.

@davidpanderson
Copy link
Contributor Author

Either name is fine. Probably not EXTRA_HEAD though.

@TheAspens
Copy link
Member

I think GLOBAL_HEAD_EXTRA make a good distinction between the other mechanism and this and will make it easier for someone in the future reading the code and deciding how to use the feature. If you could make that change then I will merge it. Thanks!

@TheAspens
Copy link
Member

@davidpanderson - can you review my last request for a change? I think it can be merged after that. Thanks.

@davidpanderson
Copy link
Contributor Author

done

@TheAspens
Copy link
Member

Thanks for the update and contribution!

@TheAspens TheAspens merged commit 073be4b into master Jan 10, 2019
@ChristianBeer ChristianBeer deleted the dpa_page_head branch February 2, 2019 10:12
@AenBleidd AenBleidd added this to the Server Release 1.2.0 milestone Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheAspens TheAspens Awaiting requested review from TheAspens

Assignees

@TheAspens TheAspens

Labels
C: Web - Project
Projects
None yet
Milestone
Server Release 1.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@davidpanderson @TheAspens @Rytiss @AenBleidd