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

#328: add flourish analytics to pages with embeds. #344

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

komejo
Copy link
Contributor

@komejo komejo commented Jan 8, 2025

What issue(s) does this solve?

Flourish events should have extended analytics

What is the new behavior?

  • Adds Flourish JS code to the when there is an embed in the body field.

Profile requirements:

  • Does deploying this change require a change to config at the site level (choose one)?
    • No config change is required.
    • Yes, and I have written an update hook to apply these config changes.
    • Yes, and I've included updating instructions to be added to the release notes. The next release will need to be a major version increase. (Only do this in special cases.)

Site-level pull requests for testing. Only merge when these PRs are approved:

Create or update any site-level pull requests following the documentation

Checked on develop (TA to do)

  • Brasil
  • China
  • Indonesia

Copy link
Collaborator

@mariacha mariacha left a comment

Choose a reason for hiding this comment

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

The code works great on embeds in the body and doesn't fire when there are no embeds on the body, so that's all working as expected! I did think of alternative cases that I didn't pass on in the first place, but I think the solution I put inline in the code should work.

@@ -45,6 +46,44 @@ function wri_seo_page_attachments(array &$attachments) {
break;

}

// Check for Flourish embeds.
if ($node->hasField('body') && !$node->get('body')->isEmpty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Limiting this to the body field won't work in all cases because sometimes embeds are possible as other fields:
https://pr-1263-wriflagship.pantheonsite.io/data/world-greenhouse-gas-emissions-2021

But if you move the embed code to be at the media level and target the field_media_embed_code field, it should work in all cases. You could use the existing functions in wri_media -- either wri_media_preprocess_field or wri_media_preprocess_media (there's already some stuff in there about field_media_embed_code in that second one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes! I like checking wri_media_preprocess_media() - it feels more lean than slogging through all the body fields. I moved everything into wri_media to keep things tidy and removed the .libraries.yml entry b/c I'm doing a direct embed of the script into the head.

@komejo komejo self-assigned this Jan 14, 2025
@komejo komejo assigned mariacha and unassigned komejo Jan 14, 2025
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.

2 participants