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.

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

Add event tags to top and bottom of body #17333

Merged
merged 1 commit into from Apr 11, 2021
Merged

Add event tags to top and bottom of body #17333

merged 1 commit into from Apr 11, 2021

Conversation

MHarmony
Copy link
Contributor

Description:

I am currently developing a plugin that appends html to the top and bottom of the body. These two tags would be helpful in my plugin development.

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@sgiehl sgiehl added the Needs Review PRs that need a code review label Mar 11, 2021
@tsteur tsteur added this to the 4.3.0 milestone Mar 11, 2021
@sgiehl
Copy link
Member

sgiehl commented Mar 12, 2021

Hi @MHarmony thanks for creating the PR. The changes are looking fine.

@tsteur could you maybe have a quick look as well. Not sure if should add those new events or not. In general we should maybe try to avoid events where possible, as they might make Matomo slower and slower if we add too many. But guess those global events might be helpful at some point maybe.

@tsteur
Copy link
Member

tsteur commented Mar 14, 2021

I think it be fine to add them 👍 What would always be helpful be to know how/for what they will be used @MHarmony . This will make it easier if/when there are changes/refactorings/... and what could break when making changes in the future there. If you can't tell us it's fine but it be good to know for us since for these twig events we don't consider API and we'd still want to make sure when/if there are any changes in the future to not break anything.

@MHarmony
Copy link
Contributor Author

It's for notices at top/bottom of pages.

@tsteur
Copy link
Member

tsteur commented Mar 22, 2021

fyi @MHarmony I just documented our notifications classes. Just wondering if this might maybe work too? https://github.com/matomo-org/developer-documentation/pull/459/files

@MHarmony
Copy link
Contributor Author

@tsteur Unfortunately not. The notices are meant to be permanent, not toasts. Like a banner of sorts.

@tsteur
Copy link
Member

tsteur commented Mar 22, 2021

The notifications can be permanent too if triggered on every page load but all good. Just thought I mention it 👍

@MHarmony
Copy link
Contributor Author

MHarmony commented Mar 22, 2021

@tsteur Thanks for the mention. I didn't know about these so this gives me something to use in the future even if it doesn't quite fit what I need here.

@flamisz flamisz mentioned this pull request Apr 7, 2021
10 tasks
@diosmosis diosmosis merged commit 375af64 into matomo-org:4.x-dev Apr 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants