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

MWPW-135821 introduce mep custom action & use it for card collection #2152

Merged
merged 6 commits into from
Apr 23, 2024

Conversation

npeltier
Copy link
Contributor

@npeltier npeltier commented Apr 16, 2024

  • and inaugurating first custom action for replacing cards in collection

Resolves: MWPW-135821

with following manifest added to the page

action selector page filter (optional) all
replace in-block:merch-card-collection   /drafts/npeltier/fragments/merch/products/catalog/photoshop/promo

before & after personalisation (see photoshop card)
Test URLs:

- and inaugurating first custom action for replacing cards in collection
@npeltier npeltier added run-nala Run Nala Test Automation against PR commerce needs-verification PR requires E2E testing by a reviewer labels Apr 16, 2024
@npeltier npeltier requested review from a team as code owners April 16, 2024 14:46
@npeltier npeltier changed the title MWPW-135821 introduce custom action MWPW-135821 introduce mep custom action & use it for card collection Apr 16, 2024
@@ -51,6 +51,10 @@ const DATA_TYPE = {
TEXT: 'text',
};

export const MERCH_CARD_COLLECTION_SELECTOR = 'in-card-collection';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this constant should not be here, personalization.js should not know about blocks needing delayed MEP based personalization.

@@ -51,6 +51,10 @@ const DATA_TYPE = {
TEXT: 'text',
};

export const MERCH_CARD_COLLECTION_SELECTOR = 'in-card-collection';

const CUSTOM_SELECTORS = [MERCH_CARD_COLLECTION_SELECTOR];
Copy link
Contributor

Choose a reason for hiding this comment

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

this too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the smallest footprint to have to get "custom" actions defined so mep knows it just needs to register them as such.

Copy link
Contributor

Choose a reason for hiding this comment

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

If selectors had a pattern or prefix, we wouldn't need to add custom selectors in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @yesil is right. The more I think about it, the more I think we should do a prefix and maybe the block name. Something like:
in-block:blockname

Copy link
Contributor

Choose a reason for hiding this comment

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

if we did that, you wouldn't need the constant. You just extract the 2nd part of the string and in your block you just use your own block name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure :) or custom:whatever but yes in that case in-block:merch-card-collection sounds right indeed :)

@@ -289,6 +293,14 @@ function getSection(rootEl, idx) {
: rootEl.querySelector(`:scope > div:nth-child(${idx})`);
}

function registerCustomAction(cmd) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we differentiate normal MEP rules from these ones?
if we expose all the MEP rules and have a flag when they are successfully executed at page load, then blocks such as merch-card-collection can go over the rules without that flag and find matching rules and execute them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they are, all normal processing is stopped if identified action is custom, and then put in config.mep.custom

Copy link
Contributor

Choose a reason for hiding this comment

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

actually I meant, do we need to differentiate, sorry for the language.

@@ -161,7 +161,23 @@ describe('Merch Cards', async () => {

it('should override cards when asked to', async () => {
const el = document.getElementById('multipleFilters');
el.dataset.overrides = '/override-photoshop,/override-express';
setConfig({
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest putting MEP rules in own config object.
it can be a module scoped config and be exposed from personalization.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is what we do, in config.mep.custom

Copy link
Contributor

Choose a reason for hiding this comment

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

I see 👍

Copy link
Contributor

@yesil yesil left a comment

Choose a reason for hiding this comment

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

In my opinion, personalization.js should not know about blocks needing to perform MEP during block initialisation.

Otherwise, the current code looks concise and simple.

@@ -289,6 +293,14 @@ function getSection(rootEl, idx) {
: rootEl.querySelector(`:scope > div:nth-child(${idx})`);
}

function registerCustomAction(cmd) {
Copy link
Contributor

Choose a reason for hiding this comment

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

actually I meant, do we need to differentiate, sorry for the language.

@@ -161,7 +161,23 @@ describe('Merch Cards', async () => {

it('should override cards when asked to', async () => {
const el = document.getElementById('multipleFilters');
el.dataset.overrides = '/override-photoshop,/override-express';
setConfig({
Copy link
Contributor

Choose a reason for hiding this comment

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

I see 👍

setConfig({
...conf,
mep: {
custom: {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: deferred or lazy instead of custom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me it's less talkative than custom. custom means in that context that it will not be handled by perso, which implies it's lazy/deferred, but also that something else should take care of it :)

@@ -51,6 +51,10 @@ const DATA_TYPE = {
TEXT: 'text',
};

export const MERCH_CARD_COLLECTION_SELECTOR = 'in-card-collection';

const CUSTOM_SELECTORS = [MERCH_CARD_COLLECTION_SELECTOR];
Copy link
Contributor

Choose a reason for hiding this comment

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

If selectors had a pattern or prefix, we wouldn't need to add custom selectors in this file.

@npeltier
Copy link
Contributor Author

npeltier commented Apr 16, 2024

If selectors had a pattern or prefix, we wouldn't need to add custom selectors in this file.

i suggested "custom-" prefix for actions :) but @vgoodric was preferring custom selectors :)

Copy link
Contributor

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

@@ -291,8 +292,7 @@ export default async function init(el) {
}

const cardsRoot = await cardsRootPromise;
const overrides = el.dataset[OVERRIDE_PATHS];
const overridePromises = overrides?.split(',').map(fetchOverrideCard);
const overridePromises = mep?.custom?.[MERCH_CARD_COLLECTION_SELECTOR]?.map(fetchOverrideCard);
Copy link
Contributor

Choose a reason for hiding this comment

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

look in personalization.js for the handleFragmentCommand function. In particular, this line:
if (config.mep.preview) a.dataset.manifestId = manifestPath;

This is what makes the highlight feature possible. I recommend doing this as well so people can see what has been changed.

@npeltier
Copy link
Contributor Author

thanks for the great suggestion about prefixes @yesil @vivgoodrich, just updated

@vgoodric
Copy link
Contributor

thanks for the great suggestion about prefixes @yesil @vivgoodrich, just updated

Can you also update your sample manifests? So I can see it all in action?

@npeltier
Copy link
Contributor Author

just did @vivgoodrich , will now tackle your 2nd suggestion

Copy link
Contributor

aem-code-sync bot commented Apr 17, 2024

Page Scores Audits Google
/?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@3ch023
Copy link
Contributor

3ch023 commented Apr 18, 2024

do we have a task to cover it with nala tests?
cc @afmicka

@afmicka
Copy link
Contributor

afmicka commented Apr 18, 2024

do we have a task to cover it with nala tests? cc @afmicka

@3ch023 not as jira. I am working on updating promo tests today and tomorrow (all actions on fragments turned out to be supported but not working) and had this in mind too. We can create the jira task for this as well. cc: @npeltier

Copy link
Contributor

This PR is currently in the needs-verification state. Please assign a QA engineer to verify the changes.

@afmicka afmicka added verified PR has been E2E tested by a reviewer and removed needs-verification PR requires E2E testing by a reviewer labels Apr 23, 2024
@vladen vladen merged commit d21f705 into adobecom:stage Apr 23, 2024
20 of 21 checks passed
@vladen vladen mentioned this pull request Apr 23, 2024
joaquinrivero added a commit to joaquinrivero/milo that referenced this pull request Apr 25, 2024
* stage:
  MWPW-146999: kodiak CVE 25883 (adobecom#2183)
  MWPW-144549 CTA alignment for Text, Icon, and Media Blocks (adobecom#2098)
  MWPW-146494-keep gnav sticky when branch banner is displayed (adobecom#2175)
  MWPW-135821 introduce mep custom action & use it for card collection (adobecom#2152)
  MWPW-146129 [Original: adobecom#2123] App Launcher overlaps the menu in Unav in the devices (adobecom#2184)
  Revert "MWPW-146129 App Launcher overlaps the menu in Unav in the devices" (adobecom#2182)
  MWPW-146129 App Launcher overlaps the menu in Unav in the devices (adobecom#2123)
  MWPW-139842 [Revert] Fill button style (adobecom#2181)
  Mwpw 142003: Mini Compare Chart card fixes (adobecom#2093)
  MWPW-146756] Add support for RTL in aside notifications, horizontal cards"" (adobecom#2178)
  Revert "[MWPW-146756] Add support for RTL in aside notifications, horizontal cards" (adobecom#2177)
  [MWPW-146756] Add support for RTL in aside notifications, horizontal cards (adobecom#2167)
  Revert "MWPW-142590 - [Share] enhancement - ability to edit text above icons" (adobecom#2172)
mokimo pushed a commit to mokimo/milo that referenced this pull request Apr 30, 2024
…dobecom#2152)

* MWPW-135821 introduce custom action

- and inaugurating first custom action for replacing cards in collection

* MWPW-135821 use prefix in selector

* MWPW-135821 adding preview data

---------

Co-authored-by: Vivian A Goodrich <101133187+vgoodric@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commerce Ready for Stage run-nala Run Nala Test Automation against PR verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants