Skip to content

Conversation

@iacore
Copy link

@iacore iacore commented Jun 12, 2024

deno task test tests/inline.test.ts now pass.

the svg element <path/> is rendered as <path></path> instead. I hope this isn't an issue.

)
.flatMap((template) =>
Array.from(template.content.querySelectorAll(selector))
Array.from(template.querySelectorAll(selector))
Copy link
Author

Choose a reason for hiding this comment

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

this usage may or may not be unique to linkedom.

Choose a reason for hiding this comment

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

I think it is - and looks like it's broken, according to https://developer.mozilla.org/en-US/docs/Web/API/HTMLTemplateElement.

I wonder if we ought to do something like the following to deal with this scenario...

template.content?.querySelectorAll(selector) || template.querySelectorAll(selector)

Choose a reason for hiding this comment

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

Actually, not sure that works...

Choose a reason for hiding this comment

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

This is a bit more robust than a previous suggestion...

Suggested change
Array.from(template.querySelectorAll(selector))
{
let inlineElements = Array.from(template.content.querySelectorAll(selector));
// Test for Linkedom
if(inlineElements.map(x => x.parentElement).filter(x => x === null).length > 0) {
inlineElements = Array.from(template.querySelectorAll(selector));
};
return inlineElements;
}

Copy link
Author

Choose a reason for hiding this comment

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

I don't think the code is correct actually, since template.content will move the children nodes to a fragment in linkedom.

One workaround is to do

template.childNodes.length === 0 ? template.content.querySelectorAll(_) : template.querySelectorAll(_)

I've talked to @WebReflection about this: WebReflection/linkedom#279

Choose a reason for hiding this comment

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

because template.content moves children indeed (once) it's a matter of tradeoffs:

  • are you sure no .content has been accessed previously? use template.qSA
  • do you want to move content ? always reach content
  • do you expect changes to be applied? I still need to understand the use case but this might be on LinkeDOM side of affairs, it's just that I have zero time these days to look at it, I am afraid

Copy link
Author

Choose a reason for hiding this comment

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

what is template.qSA?

i can't find it in the source code. we use the TypeScript version directly.

Choose a reason for hiding this comment

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

qSA is template.querySelectorAll, sorry for the terseness

@iacore
Copy link
Author

iacore commented Jun 19, 2024

forgot this was my PR :rofl_facepalm:
pushed the change now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants