feature: rewrite markup out of pipeline to ensure all block div's contain appropriate paragraph tags #45
Conversation
dist/aem.js
Outdated
| // eslint-disable-next-line import/prefer-default-export | ||
| function addMissingParagraphs(block) { | ||
| block.querySelectorAll(':scope > div > div').forEach((blockColumn) => { | ||
| const hasContent = !!blockColumn.firstElementChild || !!block.textContent.trim(); |
There was a problem hiding this comment.
| const hasContent = !!blockColumn.firstElementChild || !!block.textContent.trim(); | |
| const hasContent = blockColumn.hasChildNodes() && (!blockColumn.firstElementChild || !blockColumn.querySelector('picture')); |
There was a problem hiding this comment.
I moved the picture check into the hasWrapper check rather than hasContent but otherwise this is (I think) mostly what you suggested here.
| <div class="code original"> | ||
| <div> | ||
| <div><code>code blocks also get wrapped</code></div> | ||
| <div>text with some <code>inline code block also get wrapped</code></div> |
There was a problem hiding this comment.
one callout on code elements: In one revision of this, I was excluding code from being wrapped similar to picture but decided against it because of the possibility a text node with a <code> sibling as shown here.
If we wanted to get surgical, we could probably only wrap the code elements when they do have a text sibling, but that would add some complexity to the logic that I'm not sure is necessary.
There was a problem hiding this comment.
@shsteimer please have a look at adobe-rnd/aem-boilerplate-xwalk#2
I used a slightly different approach that only works on the DOM without requiring computed styles.
dist/aem.js
Outdated
| if (blockColumn.hasChildNodes()) { | ||
| const hasWrapper = !!blockColumn.querySelector('picture') | ||
| || /* exclude certain elements from being wrapped */ (!!blockColumn.firstElementChild | ||
| && window.getComputedStyle(blockColumn.firstElementChild).display === 'block'); |
There was a problem hiding this comment.
we should not use window.getComputedStyle() as that would allow stylesheets to interfere with the logic, e.g. setting an <a> to display: block would cause it not to be wrapped in a <p>.
dist/aem.js
Outdated
| || /* exclude certain elements from being wrapped */ (!!blockColumn.firstElementChild | ||
| && window.getComputedStyle(blockColumn.firstElementChild).display === 'block'); | ||
| if (!hasWrapper) { | ||
| const p = document.createElement('p'); |
There was a problem hiding this comment.
There are case where we would have to wrap elements in a <pre> instead, e.g. if you have a single code child in the cell. This would at least be equivalent to the default content.
…l block div's contain appropriate wrapper paragraph tags
shsteimer
force-pushed
the
pipeline-utils
branch
from
2523dd3 to
960366d
Compare
|
🎉 This PR is included in version 1.5.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
This change adds paragraph tags to block divs during decoration to avoid situations where the DOM lacks consistency depending on exactly how the block was authored.
Related Issue
Fix #44
Motivation and Context
Ideally this type of change could be made to HTML pipeline, but we can't do that for backwards compatibility reasons. By doing this here, we can slowly phase out projects that may be backwards incompatible with this type of change, and eventually (maybe?) remove it from here and add it to the pipeline.
This is likely not the only change that will eventually go into this category, which is why I did this in a new pipeline-utils file.
How Has This Been Tested?
Added test coverage. I have used similar code on projects as well
Screenshots (if appropriate):
n/a
Types of changes
Checklist: