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

bug/issue 170 handle support for element properties #184

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

briangrider
Copy link
Contributor

Related Issue

Resolves #170

Summary of Changes

  • Simplified how node properties are assigned to a fresh initialized element which allows for element properties without having to change the API
  • Added a test case for setting element properties with a custom renderer

Loosely related

  • Removed excessive commenting in dom-shim
  • Added a test case for setAttribute

…lement which allows for element properties without having to change API

- Removed excessive commenting in dom-shim
- Added a test case for setting element properties with a custom renderer
- Added a test case for setAttribute
Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Couple minor comments, but also wondering if there's anything we would want to document as part of this, or is this just kind of BAU (business as usual)?

Either way, let me do the usual Greenwood regression test run on this and will report back in a few. 🫡

@@ -161,11 +160,7 @@ async function initializeCustomElement(elementURL, tagName, node = {}, definitio
if (element) {
const elementInstance = new element(data); // eslint-disable-line new-cap

elementInstance.childNodes = childNodes;
Copy link
Member

Choose a reason for hiding this comment

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

amazing 😊


before(async function () {
const { html } = await renderToString(new URL('./src/index.js', import.meta.url));
console.log(html);
Copy link
Member

Choose a reason for hiding this comment

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

👀 😉

@@ -14,7 +14,7 @@ function isShadowRoot(element) {

function deepClone(obj, map = new WeakMap()) {
if (obj === null || typeof obj !== 'object') {
return obj; // Return primitives or functions as-is
return obj;
Copy link
Member

Choose a reason for hiding this comment

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

Just to confirm, are all the changes here just removing comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is correct!

@thescientist13 thescientist13 changed the title Added working element properties bug/issue 170 handle support for element properties Jan 20, 2025
@thescientist13 thescientist13 added the bug Something isn't working label Jan 20, 2025
Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Although GitHub actions don't seem to be running on ProjectEvergreen/greenwood#1340, at least locally this latest patch test is working and all Greenwood test cases are passing, and the website is looking good too!

I think just clean up that console log and we are good to go here.

@briangrider
Copy link
Contributor Author

Couple minor comments, but also wondering if there's anything we would want to document as part of this, or is this just kind of BAU (business as usual)?

Either way, let me do the usual Greenwood regression test run on this and will report back in a few. 🫡

I couldn't think of a reason to add documentation for this except for maybe pointing to the related test case. The only other thing I could think of, and it feels like it's out of the scope of general WCC documentation, is to point out that when writing a renderer that will be used by WCC, this is the (loose) pattern to go with:

const parsedContent = parseFragment(content);
const template = document.createElement('template');
template.content.childNodes = parsedContent.childNodes;
container.appendChild(template.content.cloneNode(true));

And innerHTML should be avoided unless you don't care about nodes retaining their properties. Maybe also mentioning that parse5 is used under the hood and is the recommended way to write a renderer that assigns properties.

I just couldn't find a way to fit those ideas into the general documentation as they seem a bit abstract and separate from the main concepts expressed there.

In the future, once I've finished my framework and documented how to integrate it with WCC, I think I might have a clearer vision here of things I'd recommend adding to the docs.

@thescientist13 thescientist13 merged commit 60b508d into master Jan 20, 2025
12 checks passed
@thescientist13 thescientist13 deleted the feature/issue-170-adding-element-props branch January 20, 2025 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue Setting Element Properties
2 participants