[MWPW-162495] : Temporary fix to handle the extra <p> generated in HTML from Helix #3412

sharmeeatadobe · 2025-01-02T09:45:58Z

Temporary fix to handle the extra <p> generated in HTML from Helix

Resolves: Multiple

Aside block

MWPW-162806

Test URLs:

Brick block

MWPW-162495

Test URLs:

Merch-card and Merch-offer-select

MWPW-162799

Test URLs:

aem-code-sync · 2025-01-02T09:46:17Z

	Page	Scores	Audits	Google
📱	/fragments/consonant/aside/aisde-split-lg-product-icons?martech=off
🖥️	/fragments/consonant/aside/aisde-split-lg-product-icons?martech=off
📱	/fragments/consonant/brick/brick-kitchen-sink-3?martech=off
🖥️	/fragments/consonant/brick/brick-kitchen-sink-3?martech=off
📱	/fragments/consonant/merch/merch-offer-selection?martech=off
🖥️	/fragments/consonant/merch/merch-offer-selection?martech=off

codecov · 2025-01-02T09:46:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (cf4c08b) to head (92a3aa1).
Report is 83 commits behind head on stage.

Additional details and impacted files

@@            Coverage Diff             @@
##            stage    #3412      +/-   ##
==========================================
+ Coverage   96.53%   98.39%   +1.86%     
==========================================
  Files         274       79     -195     
  Lines       61849    10149   -51700     
==========================================
- Hits        59705     9986   -49719     
+ Misses       2144      163    -1981

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

github-actions · 2025-01-03T01:36:50Z

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.

libs/blocks/aside/aside.js

overmyheadandbody

I'm not loving the change, I'd rather find a way to get the Helix fix merged in. Could we get someone from Experience League to check the Helix fix? Can we get someone from the Helix team to look at this?

Approving, since this currently seems like the lesser of evlis, but it's definitely not ideal.

overmyheadandbody · 2025-01-13T09:08:38Z

libs/blocks/merch-card/merch-card.js

@@ -451,6 +451,20 @@ const addStartingAt = async (styles, merchCard) => {

 export default async function init(el) {
  if (!el.querySelector(INNER_ELEMENTS_SELECTOR)) return el;
+  // TODO: Remove after bugfix PR adobe/helix-html2md#556 is merged
+  const liELs = el.querySelector('ul')?.querySelectorAll('li');


Couldn't you use el.querySelectorAll('ul li') directly?

Pushed the code change, please review

robert-bogos · 2025-01-13T09:33:30Z

libs/blocks/brick/brick.js

+    const pElements = liEl.querySelectorAll('p');
+    pElements.forEach((pElement) => {


Nit: no need to cache in here

Suggested change

const pElements = liEl.querySelectorAll('p');

pElements.forEach((pElement) => {

liEl.querySelectorAll('p').forEach((pElement) => {

Pushed the code change, please review

robert-bogos · 2025-01-13T09:34:14Z

libs/blocks/merch-card/merch-card.js

+      const pElements = liEl.querySelectorAll('p');
+      pElements.forEach((pElement) => {


Nit: same as the comment from brick.js

Suggested change

const pElements = liEl.querySelectorAll('p');

pElements.forEach((pElement) => {

liEl.querySelectorAll('p').forEach((pElement) => {

Pushed the code change, please review

sharmeeatadobe · 2025-01-13T09:47:27Z

I'm not loving the change, I'd rather find a way to get the Helix fix merged in. Could we get someone from Experience League to check the Helix fix? Can we get someone from the Helix team to look at this?

Approving, since this currently seems like the lesser of evlis, but it's definitely not ideal.

Agree to this 100%. We were not keen on making the change from our end since this is too hacky and unnecessary code change in the blocks. The Helix PR getting merged will be the best approach to solve the issue.
@adeshmukh-1585 @narcis-radu @mokimo @praveenrv

mokimo · 2025-01-13T09:49:32Z

Yeah, we should not merge this for now and observe adobe/helix-html2md#556 - but get this fix in place for when we absolutely need to merge it. So approvals & sign-offs form QA, but hold off on merging.

narcis-radu · 2025-01-13T14:35:09Z

@sharmeeatadobe - thank you for providing the temporary fix. As you, @overmyheadandbody and @mokimo suggested, we should only merge this if it's absolutely necessary. Let's get it properly tested, @ancy28sr , @SilviuLCF though.
If adobe/helix-html2md#556 is not merged, we can think about merging this one instead.
I will request changes to make sure the code is not picked up after we have signoff from QA.

narcis-radu

Requesting changes to block the automated workflow. See thread for details.

github-actions · 2025-01-21T01:24:17Z

This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label.

mokimo · 2025-01-21T09:00:35Z

Not stale.

github-actions · 2025-01-29T01:24:13Z

This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label.

mokimo · 2025-01-29T10:11:04Z

Not stale, however we should rebase to stage and ensure the branch is green from the unit tests / nala tests

sharmeeatadobe · 2025-01-31T05:47:05Z

UTs are successful in local run. Need a job re-run in this PR.

github-actions · 2025-02-08T01:23:47Z

This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label.

github-actions · 2025-02-18T01:26:19Z

This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label.

github-actions · 2025-02-25T01:36:50Z

Closing this PR due to inactivity.

…ugfix

sharmeeatadobe changed the title ~~[MWPW-162495][MWPW-162495] : Temporary fix to handle the extra <p> generated in HTML from Helix~~ [MWPW-162495] : Temporary fix to handle the extra <p> generated in HTML from Helix Jan 8, 2025

sharmeeatadobe requested a review from a team as a code owner January 8, 2025 11:50

sharmeeatadobe requested a review from a team January 9, 2025 04:38

mokimo reviewed Jan 10, 2025

View reviewed changes

libs/blocks/aside/aside.js Show resolved Hide resolved

overmyheadandbody approved these changes Jan 13, 2025

View reviewed changes

robert-bogos approved these changes Jan 13, 2025

View reviewed changes

narcis-radu self-requested a review January 13, 2025 14:35

narcis-radu requested changes Jan 13, 2025

View reviewed changes

github-actions bot added the Stale label Jan 21, 2025

mokimo removed the Stale label Jan 21, 2025

github-actions bot added the Stale label Jan 29, 2025

mokimo removed the Stale label Jan 29, 2025

sharmeeatadobe added 8 commits January 31, 2025 10:54

Fix for brick block

29994df

Fix for aside block

97e894f

Updated mocks for UT coverage

e8061f4

Updated todo comments

c278cdf

[MWPW-162799] - fix for radio-buttons in merch-card

19aff30

Null check for li elements in merch-card

d428baf

Addressed review comments

e77ee74

Addressed review comments for pElements

92a3aa1

sharmeeatadobe force-pushed the helix5bugfix branch from 87b0dd1 to 92a3aa1 Compare January 31, 2025 05:24

github-actions bot added the Stale label Feb 8, 2025

mokimo removed the Stale label Feb 10, 2025

github-actions bot added the Stale label Feb 18, 2025

github-actions bot closed this Feb 25, 2025

mokimo reopened this Feb 25, 2025

Merge branch 'stage' of https://github.com/adobecom/milo into helix5b…

89263fa

…ugfix

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MWPW-162495] : Temporary fix to handle the extra <p> generated in HTML from Helix #3412

[MWPW-162495] : Temporary fix to handle the extra <p> generated in HTML from Helix #3412

sharmeeatadobe commented Jan 2, 2025 •

edited

Loading

aem-code-sync bot commented Jan 2, 2025 •

edited

Loading

codecov bot commented Jan 2, 2025 •

edited

Loading

github-actions bot commented Jan 3, 2025

overmyheadandbody left a comment

overmyheadandbody Jan 13, 2025

sharmeeatadobe Jan 13, 2025

robert-bogos Jan 13, 2025

sharmeeatadobe Jan 13, 2025

robert-bogos Jan 13, 2025

sharmeeatadobe Jan 13, 2025

sharmeeatadobe commented Jan 13, 2025

mokimo commented Jan 13, 2025 •

edited

Loading

narcis-radu commented Jan 13, 2025

narcis-radu left a comment

github-actions bot commented Jan 21, 2025

mokimo commented Jan 21, 2025

github-actions bot commented Jan 29, 2025

mokimo commented Jan 29, 2025

sharmeeatadobe commented Jan 31, 2025

github-actions bot commented Feb 8, 2025

github-actions bot commented Feb 18, 2025

github-actions bot commented Feb 25, 2025

		const pElements = liEl.querySelectorAll('p');
		pElements.forEach((pElement) => {

	const pElements = liEl.querySelectorAll('p');
	pElements.forEach((pElement) => {
	liEl.querySelectorAll('p').forEach((pElement) => {

[MWPW-162495] : Temporary fix to handle the extra <p> generated in HTML from Helix #3412

Are you sure you want to change the base?

[MWPW-162495] : Temporary fix to handle the extra <p> generated in HTML from Helix #3412

Conversation

sharmeeatadobe commented Jan 2, 2025 • edited Loading

Aside block

Brick block

Merch-card and Merch-offer-select

aem-code-sync bot commented Jan 2, 2025 • edited Loading

codecov bot commented Jan 2, 2025 • edited Loading

Codecov Report

github-actions bot commented Jan 3, 2025

overmyheadandbody left a comment

Choose a reason for hiding this comment

overmyheadandbody Jan 13, 2025

Choose a reason for hiding this comment

sharmeeatadobe Jan 13, 2025

Choose a reason for hiding this comment

robert-bogos Jan 13, 2025

Choose a reason for hiding this comment

sharmeeatadobe Jan 13, 2025

Choose a reason for hiding this comment

robert-bogos Jan 13, 2025

Choose a reason for hiding this comment

sharmeeatadobe Jan 13, 2025

Choose a reason for hiding this comment

sharmeeatadobe commented Jan 13, 2025

mokimo commented Jan 13, 2025 • edited Loading

narcis-radu commented Jan 13, 2025

narcis-radu left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 21, 2025

mokimo commented Jan 21, 2025

github-actions bot commented Jan 29, 2025

mokimo commented Jan 29, 2025

sharmeeatadobe commented Jan 31, 2025

github-actions bot commented Feb 8, 2025

github-actions bot commented Feb 18, 2025

github-actions bot commented Feb 25, 2025

sharmeeatadobe commented Jan 2, 2025 •

edited

Loading

aem-code-sync bot commented Jan 2, 2025 •

edited

Loading

codecov bot commented Jan 2, 2025 •

edited

Loading

mokimo commented Jan 13, 2025 •

edited

Loading