Skip to content

Add template file#6

Merged
iMattPro merged 8 commits intophpbb-extensions:masterfrom
Pico:temp-event
Jul 16, 2014
Merged

Add template file#6
iMattPro merged 8 commits intophpbb-extensions:masterfrom
Pico:temp-event

Conversation

@Pico
Copy link
Copy Markdown
Contributor

@Pico Pico commented Jul 14, 2014

No description provided.

@Pico Pico changed the title [WIP] Add template file Add template file Jul 14, 2014
@iMattPro
Copy link
Copy Markdown
Contributor

There is already a PR working on the event listener.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, there is no enable setting. Empty GOOGLEANALYTICS_ID var means not enabled.
<!-- IF GOOGLEANALYTICS_ID -->

@Pico
Copy link
Copy Markdown
Contributor Author

Pico commented Jul 14, 2014

Updated

Comment thread event/listener.php Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think core.page_header is a better location?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any diffrence?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

core.common probably isn't the best, $user isn't setup yet. We don't need $user right now, but if others copy this they may have problems.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just figured page_header is a logical choice since we are adding something to the head, and page_header is called for every real page.

@Pico
Copy link
Copy Markdown
Contributor Author

Pico commented Jul 15, 2014

Updated

@iMattPro
Copy link
Copy Markdown
Contributor

Looks great!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the script be indented one tab?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. Once it is tabbed, another time it is not tabbed...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it probably should, especially since this is an IF conditional structure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine like this as the if and script tags encompass the whole file and quite often we don't indent for if statements in templates.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then this is good to merge IMO

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hit the green button then. 🚢

iMattPro added a commit that referenced this pull request Jul 16, 2014
@iMattPro iMattPro merged commit aef8eea into phpbb-extensions:master Jul 16, 2014
@iMattPro iMattPro added this to the Beta 1 Release milestone Jul 20, 2014
iMattPro pushed a commit that referenced this pull request Jun 12, 2020
Cube707 referenced this pull request in Cube707/phpbb-matomoanalytics Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants