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

Added support for AJO click tracking. #968

Merged
merged 4 commits into from
Mar 13, 2023
Merged

Added support for AJO click tracking. #968

merged 4 commits into from
Mar 13, 2023

Conversation

XDex
Copy link
Member

@XDex XDex commented Feb 24, 2023

Description

Support AJO click-tracking

In cases when for the same click we have multiple AJO click-tracking selectors on nested DOM elements matching, we need to include just the label of the innermost matching click-track in the outgoing interact event, i.e. the one closest to the clicked element in the DOM tree.
As in the current click collection implementation we're interating through all selectors from the local click-tracking cache trying to match each of them to the clicked element DOM path, we need a way of determining the innermost matching click-track. In order to accomplish this, we're assigning a weight to each matched AJO click-track with a label, which then enables us to determine the label to be included in the event as the one with the lowest weight (i.e. closest to the clicked-element).

Related Issue

https://jira.corp.adobe.com/browse/CJM-42163

Motivation and Context

Support AJO click-tracking, see more details in the Wiki

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change which does not add functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA or I'm an Adobe employee.
  • I have made any necessary test changes and all tests pass.
  • I have run the Sandbox successfully.

Copy link
Contributor

@ninaceban ninaceban left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @XDex .
I have looked over the PR and there is a bit of disconnect between the design wiki and the code. The implementation could be simplified.
It would help if you could add some use case scenarios to PR description, so it's easier to follow why the implementation diverges from proposed design.

src/components/Personalization/createExecuteDecisions.js Outdated Show resolved Hide resolved

while (element && element !== documentElement) {
if (matchesSelectorWithEq(selector, element)) {
return getClickMetasBySelector(selector);
const matchedMetas = getClickMetasBySelector(selector);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just extract the first label found?
As far as I saw in the design wiki we should pick up the first label from one of the matching selectors.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't just extract the first label, since we don't know for sure whether it belongs to the innermost DOM element, or whether it's higher up the DOM tree. This is mentioned explicitly in the design wiki:

In case of multiple click-tracking selector matches, all matching propositions will be included in a single interact event, with the event propositionAction → label set to the label of the first click-tracking proposition having a label, when traversing the DOM tree from the innermost to the outermost DOM element.

@XDex XDex requested a review from ninaceban March 2, 2023 19:52
@XDex
Copy link
Member Author

XDex commented Mar 2, 2023

@ninaceban I've pushed some fixes, please take another look. Thank you!

I have looked over the PR and there is a bit of disconnect between the design wiki and the code.
...why the implementation diverges from proposed design.

Could you please clarify what exactly is diverging from the design in the implementation?

@ninaceban
Copy link
Contributor

@ninaceban I've pushed some fixes, please take another look. Thank you!

I have looked over the PR and there is a bit of disconnect between the design wiki and the code.
...why the implementation diverges from proposed design.

Could you please clarify what exactly is diverging from the design in the implementation?

My concern was the logic of extracting all the labels and setting a weight. We discussed offline about the reasoning behind the need of extracting all vs extracting first.
I'd like to recommend that when there is a specific logic outlined in the PR, it would be much easier for the reviewer to review the code when there is a description on why such steps were taken, or why this was needed, some use cases/edge cases.

@XDex XDex merged commit f4822c9 into adobe:main Mar 13, 2023
@XDex XDex deleted the CJM-42163 branch March 13, 2023 16:03
@ninaceban ninaceban changed the title CJM-42163 Support AJO click-tracking Added support for AJO click tracking Mar 27, 2023
@ninaceban ninaceban changed the title Added support for AJO click tracking Added support for AJO click tracking. Mar 27, 2023
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