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

[#1641][commerce] using parent option in createTag, as well as innerHtml sometimes #1706

Closed
wants to merge 2 commits into from

Conversation

Copy link
Contributor

aem-code-sync bot commented Jan 8, 2024

Page Scores Audits Google
/docs/library/kitchen-sink/merch-card?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Comment on lines +10 to +11
const p = createTag('p', { class: 'action-area' }, null, { parent: footer });
createTag('a', { slot: 'cta', is: 'checkout-link' }, null, { parent: p });
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how the render flow is or if it matters for your use-case, but you're appending a child to the footer, then appending another grandchild, this could lead to some minor CLS or other such issues. As in: footer is initially just <footer></footer> and already is an element added to the DOM. L10 makes it <footer><p class="action-area"></p></footer>, adds it to the DOM, causes some paints/reflows depending on styling. Then L11 makes it <footer><p class="action-area"><a slot="cta" is="checkout-link"></a></p></footer>, adds it to the DOM, causes another round of paint/reflow. It might be worth building the final element before appending it to the DOM. Same comment might be valid for L6-7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i thought about that but as no "long" action is done was not believing such CLS could happen, is it really the case?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to @overmyheadandbody points.
We should attach the DOM elements at the end of the construction of their trees.

@honstar
Copy link
Contributor

honstar commented Feb 22, 2024

@npeltier , any update on this PR?

@Blainegunn Blainegunn changed the base branch from main to stage March 1, 2024 22:54
@Blainegunn Blainegunn requested a review from a team as a code owner March 1, 2024 22:54
@npeltier
Copy link
Contributor Author

npeltier commented Mar 4, 2024

closing for now :( too many changes on commerce ahead

@npeltier npeltier closed this Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants