[#1641] add parent option to createTag #1647

npeltier · 2023-12-13T09:09:56Z

add options parameter to createTag signature, with only one parent option, as discussed in https://github.com/orgs/adobecom/discussions/1641
make usage of it in most obvious cases

Resolves: #1641

Test URLs:

…option, as discussed in https://github.com/orgs/adobecom/discussions/1641 - make usage of it in most obvious cases

aem-code-sync · 2023-12-13T09:09:59Z

Hello, I'm the AEM Code Sync Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

Re-run PSI Checks

aem-code-sync · 2023-12-13T09:10:17Z

Page	Scores	Audits	Google
/?martech=off
/docs/library/kitchen-sink/merch-card?martech=off

codecov · 2023-12-13T09:12:26Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (dba63ec) 95.67% compared to head (6a066c3) 95.02%.
Report is 14 commits behind head on main.

❗ Current head 6a066c3 differs from pull request most recent head f08e92f. Consider uploading reports for the commit f08e92f to get more accurate results

Files	Patch %	Lines
libs/blocks/featured-article/featured-article.js	0.00%	6 Missing ⚠️
libs/blocks/article-feed/article-feed.js	0.00%	2 Missing ⚠️
libs/features/personalization/preview.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1647      +/-   ##
==========================================
- Coverage   95.67%   95.02%   -0.65%     
==========================================
  Files         154      153       -1     
  Lines       39657    39562      -95     
==========================================
- Hits        37940    37592     -348     
- Misses       1717     1970     +253

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

libs/utils/utils.js

overmyheadandbody

Looking good for the most part, it must've been a tedious task to go through all of these. I just have a few notes

libs/blocks/library-config/lists/personalization.js

test/blocks/instagram/mocks/embed-utils.js

test/blocks/marketo/mocks/marketo-utils.js

test/blocks/slideshare/mocks/utils.js

libs/utils/utils.js

overmyheadandbody · 2023-12-13T12:24:11Z

Also, you could add the needs-verification label so the checks are on a better path to green

narcis-radu · 2023-12-15T18:09:20Z

libs/blocks/library-config/library-config.js

@@ -143,8 +143,7 @@ async function combineLibraries(base, supplied) {
 function createList(libraries) {
  const container = createTag('div', { class: 'con-container' });

-  const libraryList = createTag('ul', { class: 'sk-library-list' });
-  container.append(libraryList);
+  const libraryList = createTag('ul', { class: 'sk-library-list' }, null, { parent: container });


you missed one element here -

milo/libs/blocks/library-config/library-config.js

Line 151 in 694c4d1

const item = createTag('li', { class: 'content-type' }, type.replace('_', ' '));

narcis-radu · 2023-12-15T18:11:35Z

test/blocks/slideshare/mocks/utils.js

@@ -22,6 +22,9 @@ export function createTag(tag, attributes, html) {
      el.setAttribute(key, val);
    });
  }
+  if (options?.parent) {


options will always be defined, no need for ?. I think we can skip the whole if clause and do options.parent?.append(el);

yes, forgot to cleanup that test, thanks

aem-code-sync · 2023-12-18T12:47:27Z

Page	Scores	Audits	Google
/?martech=off
/docs/library/kitchen-sink/merch-card?martech=off

hparra

Sorry for delay. I was out for most of December.

hparra · 2024-01-03T20:11:25Z

libs/blocks/video-metadata/video-metadata.js

@@ -115,6 +115,5 @@ export default function init(el) {
  el.remove();
  const obj = createVideoObject(metadata);
  if (!obj) return;
-  const script = createTag('script', { type: 'application/ld+json' }, JSON.stringify(obj));
-  document.head.append(script);
+  createTag('script', { type: 'application/ld+json' }, JSON.stringify(obj), { parent: document.head });


hparra · 2024-01-03T20:11:45Z

libs/features/seotech/seotech.js

@@ -42,8 +42,7 @@ export async function appendScriptTag({ locationUrl, getMetadata, createTag, get
    ? SEOTECH_API_URL_PROD : SEOTECH_API_URL_STAGE;

  const append = (obj) => {
-    const script = createTag('script', { type: 'application/ld+json' }, JSON.stringify(obj));
-    document.head.append(script);
+    createTag('script', { type: 'application/ld+json' }, JSON.stringify(obj), { parent: document.head });


libs/utils/utils.js

hparra · 2024-01-03T20:15:00Z

test/utils/utils.test.js

@@ -572,6 +573,20 @@ describe('Utils', () => {
    });
  });

+  describe('createTag', async () => {


@npeltier did createTag not have a UT prior to this diff?

not specifically, but indirectly i think it was already 100% covered

hparra · 2024-01-03T20:17:58Z

libs/blocks/event-rich-results/event-rich-results.js

@@ -84,7 +84,6 @@ function getEvent(el) {
 export default function init(el) {
  const event = getEvent(el);
  logNullValues(event);
-  const script = createTag('script', { type: 'application/ld+json' }, JSON.stringify(event));
-  document.head.append(script);
+  createTag('script', { type: 'application/ld+json' }, JSON.stringify(event), { parent: document.head });


lgtm. not formal owner but it is seo.

auniverseaway

I would have opted to change these in batches and start with just changing the function first.

We always encourage small changes with small blast radiuses over large sweeping changes. The rare exception is net-new features that do not touch existing functionality (UAR, Merch, GlobalNav, etc.).

If one of these features blows up because there was a specific sequencing that should have been followed... appends have a very specific order, this entire PR must be reverted. Not having the complete historical knowledge of each use of create tag makes this a very risky PR.

Who is going to E2E QA this before it gets merged?

npeltier · 2024-01-04T09:41:43Z

I would have opted to change these in batches and start with just changing the function first.

We always encourage small changes with small blast radiuses over large sweeping changes. The rare exception is net-new features that do not touch existing functionality (UAR, Merch, GlobalNav, etc.).

If one of these features blows up because there was a specific sequencing that should have been followed... appends have a very specific order, this entire PR must be reverted. Not having the complete historical knowledge of each use of create tag makes this a very risky PR.

Who is going to E2E QA this before it gets merged?

this makes sense, will make that PR the "feature" one, and usage in other ones (may be one per area, will see)

npeltier · 2024-01-04T09:46:03Z

actually will close that one for simplicity reasons :)

npeltier · 2024-01-04T10:48:35Z

core one #1698

- add options parameter to createTag signature, with only one parent …

590f9a0

…option, as discussed in https://github.com/orgs/adobecom/discussions/1641 - make usage of it in most obvious cases

npeltier added run-nala Run Nala Test Automation against PR @marquee Run Marquee Tests Nala @marketo Run Marketo Block Tests @modal Run Modal Tests Nala commerce labels Dec 13, 2023

npeltier requested review from hparra, ryanmparrish, Brandon32, colloyd, sabyamon, JackySun9, elaineskpt, echampio-at-adobe, VKniaz, Axelcureno, yesil, 3ch023, Sartxi, auniverseaway and elan-tbx as code owners December 13, 2023 09:09

npeltier requested review from rgclayton and a team as code owners December 13, 2023 09:09

Merge branch 'main' of github.com:adobecom/milo into discussion_1641

190ab0f

mokimo approved these changes Dec 13, 2023

View reviewed changes

libs/utils/utils.js Outdated Show resolved Hide resolved

add default value

83be7a4

vladen approved these changes Dec 13, 2023

View reviewed changes

npeltier added 2 commits December 13, 2023 11:44

Merge branch 'main' of github.com:adobecom/milo into discussion_1641

0086fe4

obvious eslint issues

78328cb

overmyheadandbody reviewed Dec 13, 2023

View reviewed changes

review comments

89f0930

npeltier added the needs-verification PR requires E2E testing by a reviewer label Dec 13, 2023

npeltier added 5 commits December 13, 2023 13:54

remove additional append

10925b8

Merge branch 'main' of github.com:adobecom/milo into discussion_1641

f06ee0b

unnecessary check

6a066c3

Merge branch 'main' of github.com:adobecom/milo into discussion_1641

1682220

Merge branch 'main' of github.com:adobecom/milo into discussion_1641

694c4d1

overmyheadandbody approved these changes Dec 14, 2023

View reviewed changes

Axelcureno approved these changes Dec 15, 2023

View reviewed changes

narcis-radu reviewed Dec 15, 2023

View reviewed changes

npeltier added 2 commits December 18, 2023 13:46

narcis review

a3394cd

Merge branch 'main' of github.com:adobecom/milo into discussion_1641

d0c3ea0

narcis-radu approved these changes Jan 3, 2024

View reviewed changes

Merge branch 'main' into discussion_1641

330a9e7

hparra approved these changes Jan 3, 2024

View reviewed changes

JackySun9 approved these changes Jan 3, 2024

View reviewed changes

Merge branch 'main' into discussion_1641

f08e92f

auniverseaway reviewed Jan 3, 2024

View reviewed changes

npeltier closed this Jan 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#1641] add parent option to createTag #1647

[#1641] add parent option to createTag #1647

npeltier commented Dec 13, 2023

aem-code-sync bot commented Dec 13, 2023

aem-code-sync bot commented Dec 13, 2023 •

edited

Loading

codecov bot commented Dec 13, 2023 •

edited

Loading

overmyheadandbody left a comment

overmyheadandbody commented Dec 13, 2023

narcis-radu Dec 15, 2023

narcis-radu Dec 15, 2023

npeltier Dec 18, 2023

aem-code-sync bot commented Dec 18, 2023 •

edited

Loading

hparra left a comment

hparra Jan 3, 2024

hparra Jan 3, 2024

hparra Jan 3, 2024

npeltier Jan 3, 2024

hparra Jan 3, 2024

auniverseaway left a comment

npeltier commented Jan 4, 2024

npeltier commented Jan 4, 2024

npeltier commented Jan 4, 2024

[#1641] add parent option to createTag #1647

[#1641] add parent option to createTag #1647

Conversation

npeltier commented Dec 13, 2023

aem-code-sync bot commented Dec 13, 2023

aem-code-sync bot commented Dec 13, 2023 • edited Loading

codecov bot commented Dec 13, 2023 • edited Loading

Codecov Report

overmyheadandbody left a comment

Choose a reason for hiding this comment

overmyheadandbody commented Dec 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aem-code-sync bot commented Dec 18, 2023 • edited Loading

hparra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auniverseaway left a comment

Choose a reason for hiding this comment

npeltier commented Jan 4, 2024

npeltier commented Jan 4, 2024

npeltier commented Jan 4, 2024

aem-code-sync bot commented Dec 13, 2023 •

edited

Loading

codecov bot commented Dec 13, 2023 •

edited

Loading

aem-code-sync bot commented Dec 18, 2023 •

edited

Loading