-
Notifications
You must be signed in to change notification settings - Fork 15
Fix: Properly render <template> capability heading without spacing wo… #250
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
Open
Abhikorekar
wants to merge
1
commit into
ProjectEvergreen:master
Choose a base branch
from
Abhikorekar:fix-template-capability-heading
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,9 +105,9 @@ WCC aims to cover a reasonable set of DOM APIs to help facilitate server-renderi | |
| <p>Declarative Shadow DOM</p> | ||
| </wcc-capability-box> | ||
|
|
||
| <wcc-capability-box heading="< template >"> | ||
| <p>Template element support (createElement)</p> | ||
| </wcc-capability-box> | ||
| <wcc-capability-box heading="<template>"> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like formatting is off here. Prettier formatting should catch this, if you run it before committing |
||
| <p>Template element support (createElement)</p> | ||
| </wcc-capability-box> | ||
|
|
||
| <wcc-capability-box heading="CSSStyleSheet"> | ||
| <p>Constructable Stylesheets shim</p> | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per your comment in the description
Could you could expand on your solution here? I'm wondering if this is something that could / should be addressed within the JSX transformation itself. If you can provide some more details on the issue you observed, perhaps I can guide us to a solution that doesn't requiring refactoring the entire component.
Maybe it's a parse5 issue? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, doing a little playing around with AST explorer and browsing the parse5 GitHub, it does seem to be a parse5 "issue" - inikulin/parse5#191
In AST explorer, i can see

<template>is getting parsed as<template>literallySo I think the real work is not in the component itself, but rather in WCC itself, likely here
https://github.com/ProjectEvergreen/wcc/blob/0.19.0/src/wcc.js#L25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming the parse5 behavior.
After stepping back from the component-level workaround, I agree this doesn’t seem like a rendering issue inside wcc-capability-box, but rather how WCC handles parsing and serialization after parse5 decodes entities like <template> into .
Looking at src/wcc.js, we parse via the DOM shim (which uses parse5) and later serialize the transformed tree with serialize(). Since parse5 decodes entities per spec, the attribute value becomes in the AST, and it appears to be getting interpreted as markup somewhere in that flow.
Before I make further changes, would you recommend focusing on how attribute values are handled during serialization (e.g. before serialize(finalTree)), or is it better to adjust behavior in the DOM shim layer?
I just want to make sure I’m addressing this at the correct abstraction level in WCC rather than introducing another workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... that's a good question. My hunch is that the bulk of the parse5 work is happening in the DOM shim, but I don't know if there would be any sort of pre-processing needed from WCC first to understand of there are entities that might get transformed? Since the behavior seems inherent in parse5, as the linked issue suggests, maybe some handling needs to happen on the way in and out? 🤔
Honestly, what I would do personally in a situation like this is create a unit test for the behavior in the WCC test suite (and with #246 we can do the same for the capability box component itself) and I would just explore what's possible to get the desired outcome with a lot of logging and a very narrow test case. Since I'm not sure to what degree working around parse5 would entail, and without a clear understanding of the exact solution, I would probably worry first about what it would take just to get things to work, and sort it out once I've made it that far.
Probably not a great answer, but that's pretty much how it goes sometimes. So maybe start a new branch / PR there? Happy to assist and answer questions as you make progress 🙌