pinterest-for-woocommerce icon indicating copy to clipboard operation
pinterest-for-woocommerce copied to clipboard

:bulb: Idea for catching click events to fire Tracks

Open tomalec opened this issue 3 years ago • 1 comments

Is your feature request related to a problem?

This is a follow-up from the discussion at https://github.com/woocommerce/pinterest-for-woocommerce/pull/266#issuecomment-977669697, where we were looking for a better way to fire track events upon clicking on a link, without the risk of onClick prop overwrite.

How to reproduce the problem

<MyElement
	onClick={}
	{ ...documentationLinkProps() }
	onClick={}
	/>

Describe the solution you'd like

@eason9487 wrote:

a new idea might be considered, but only for this kind of event tracking, not like the generalized pattern of Component hooks. […] The idea is to use a single global click event with capture type, then it doesn't need to bind onClick prop on the target React element. So it'd not overwrite others or be overwritten by others. But the limitation is it has to be a react hook.

import { uniqueId } from 'lodash';
import { useRef, useEffect } from '@wordpress/element';
import { recordEvent } from '@woocommerce/tracks';

const DATA_ATTR = 'data-pwf-doc-link-id';
const payloadMap = new Map();

function handleClick( e ) {
	const id = e.target.getAttribute( DATA_ATTR );
	const payload = payloadMap.get( id );
	if ( payload ) {
		recordEvent( 'pfw_documentation_link_click', payload );
	}
}

export default function useDocLinkTracking( {
	href,
	linkId,
	context,
	target = '_blank',
	rel = 'noopener',
} ) {
	const idRef = useRef( null );
	if ( ! idRef.current ) {
		idRef.current = uniqueId();
	}

	useEffect( () => {
		if ( payloadMap.size === 0 ) {
			document.defaultView.addEventListener( 'click', handleClick, true );
		}

		const id = idRef.current;

		payloadMap.set( id, {
			link_id: linkId,
			context,
			href,
		} );

		return () => {
			payloadMap.delete( id );

			if ( payloadMap.size === 0 ) {
				document.defaultView.removeEventListener(
					'click',
					handleClick,
					true
				);
			}
		};
	}, [ href, linkId, context ] );

	return {
		[ DATA_ATTR ]: idRef.current,
		href,
		target,
		rel,
	};
}

Considering that registering a global click event for each tracking usages should not be a problem. So the click event could be added and removed inside the useDocLinkTracking hook, therefore, the if ( payloadMap.size === 0 ) checks and the payloadMap map are unneeded.

Additional context

tomalec avatar Nov 24 '21 13:11 tomalec

Interesting idea.

What I like here is:

  • a global approach to catch all documentation links with one module/entity,
  • native API

But there is also a number of things that bothers me:

  • On one hand, it's global, but it still applies an event handler for every link - to me it seems redundant. If we already attach the event listener to the document, that checks the shared payloadMap we could actually addEventListener it once.
  • I'm not sure how it would interact with other React-bound events. Does it always translate onClick props to the bubbling phase listeners?
  • I'd rather use ownerDocument rather than document to make sure we target the correct one.
  • I'm not sure if assigning the event listeners using native APIs is fine in a functional React world

With native custom elements, I've been using a similar thing in the past, with a code like:

<any-element-in-shadow-or-document>
	<track-clicks/>
	…
	<div>…<a href="foo" my-opt-in-attr="some-payload">Read more</a>
	…
</any-element-in-shadow-or-document>

Then my-link-catcher when connected was listening and capturing all click events in the scope of its parent node (any-element-in-shadow-or-document). Then was checking if the target element has opt-in attribute (or didn't have opt-out attribute - depending on strategy), if it was ok, it was performing an action with the given payload.

It was more-or-less smth like:

class TrackClicks extends HTMLElement {
   connectedCallback() {
   	this.parentNode.addEventListener('click', this, true);
   }
   disConnectedCallback() {
   	this.parentNode.removeEventListener('click', this);
   }
   handleEvent( event ){
   	if( event.target.hasAttiribute( 'data-pwf-doc-link-id' ) ){
   		recordEvent( 'pfw_documentation_link_click', {
   			linkId: event.target.getAttiribute( 'data-pwf-doc-link-id' ),
			context:  event.target.getAttiribute( 'data-pwf-doc-context' ),
   			href: event.target.href
   		});
   	}
   }
};

For sure it does not fit the React world, but the idea is to

  • add/remove the listener once
  • have catching declaratively scoped
  • work across shadow roots & external documents
  • easy to use - just add an attribute to your link (no need for onClick, or hook)

tomalec avatar Nov 24 '21 13:11 tomalec