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 filter to allow custom attributes on Web Stories rendering #10005

Conversation

freibergerb
Copy link
Contributor

@freibergerb freibergerb commented Dec 15, 2021

Context

We want to be able to add custom attributes to the a tag when Web Stories Widgets are added to a page.
Our use case is that we also have another Analytics platform that isn't Google Analytics and this is the way we thought to integrate.

Summary

We are adding a filter called story_render_attributes passing 3 different parameters to it, $this->current which is an instance of the Story object, its position and the get_view_type which is driven by the widget selected.

Relevant Technical Choices

We thought that a filter would be the most appropriated way to achieve that.

To-do

None I can think of at this stage but let's see what the contributors think and if they have any other ideas.

User-facing changes

None, this is transparent for users.

Testing Instructions

We do have this patch current implemented live with the filter been added on our end (outside this plugin). It's not on our best interests though to have a modified version of this plugin in our codebase as it will be a problem to update as you release new functionalities. I hope this can be useful for other use cases as well.

  • This is a non-user-facing change and requires no QA

This PR can be tested by following these steps:

Reviews

Does this PR have a security-related impact?

Does this PR change what data or activity we track or use?

Does this PR have a legal-related impact?

Checklist

  • This PR addresses an existing issue and I have linked this PR to it in ZenHub
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Fixes #

@freibergerb freibergerb marked this pull request as ready for review December 15, 2021 02:24
@swissspidy
Copy link
Collaborator

Thanks for for your contribution!

Please note that you'll need to sign the Contributor License Agreement before we could accept it.

I'm curious to learn more about this analytics solution you are using. What kind of attributes do you need to set?

For the stories embeds we actually prevent clicks on the <a> elements so we can open stories in a lightbox. Doesn't that interfere with your analytics? If so, it might be more preferable for you to add a JavaScript integration instead so you can better track things like the lightbox opening, story play/pause/mute, etc.

As for the filter, are all the arguments necessary for your integration to work? Or do you perhaps only need the story object as context?

@freibergerb
Copy link
Contributor Author

hey @swissspidy,

No worries I just signed.

Thank you for looking into this. We have Adobe Analytics as our main Analytics solution. The href would look like below:
<a href="https://www.theaustralian.com.au/web-stories/free/the-australian/taliban-gunmen-steal-australian-food-aid-in-afghanistan" data-tgev="event290" data-tgev-metric="ev" data-tgev-order="1" data-tgev-container="web-stories-carousel" data-tgev-label="Taliban gunmen steal Australian food aid in Afghanistan">

These are the exact attributes the Analytics team has passed to us and asked to be implemented. The reason why we have 3 parameters is because we do use the order + widget name/id as data-tgev-order and data-tgev-container respectively.

If you would like to see this live you can go on https://www.theaustralian.com.au/ and scroll down or search for "Visual Stories".

If I'm honest I'm unaware how Adobe Analytics work in detail and if this could be handled any other way. I'm happy to raise with the Analytics team though.

What are your main concerns around this addition? A potentially very specific use case?

Thanks!

@swissspidy
Copy link
Collaborator

swissspidy commented Dec 16, 2021

Awesome, thanks a lot for the context!

The use case does sound reasonable for sure, no concerns there 🙂

I assume you have heard from the Analytics team already that this works like intended?


I am asking because there seem to be some styling issues with the Web Stories embeds currently that prevent the clicks from being tracked properly. Something that we'll need to fix in the plugin asap. Right now the <a> tags have height of 0.

If you run document.querySelectorAll('.web-stories-list__story-poster a').forEach(link => link.addEventListener('click', console.log)) in dev tools and click on some stories, you'll see that nothing is getting logged right now because of this.

includes/Renderer/Stories/Renderer.php Outdated Show resolved Hide resolved
includes/Renderer/Stories/Renderer.php Outdated Show resolved Hide resolved
includes/Renderer/Stories/Renderer.php Outdated Show resolved Hide resolved
@swissspidy swissspidy added Group: WordPress Changes related to WordPress or Gutenberg integration Pod: WP & Infra Type: Enhancement New feature or improvement of an existing feature Group: Blocks Issues related to the provided Gutenberg Blocks labels Jan 27, 2022
@swissspidy swissspidy merged commit c5ca09d into GoogleForCreators:main Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Group: Blocks Issues related to the provided Gutenberg Blocks Group: WordPress Changes related to WordPress or Gutenberg integration Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants