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

feat: Preserve custom element tags #952

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ras0q
Copy link

@ras0q ras0q commented Jan 23, 2025

close #951

@ras0q ras0q force-pushed the feat/tags-to-preserve branch from cd3c160 to 2ca866b Compare January 23, 2025 02:03
@ras0q
Copy link
Author

ras0q commented Jan 27, 2025

@gijsk Could you please review this PR?

...this.DEFAULT_TAGS_TO_SCORE,
...this._tagsToPreserve,
];
if (tagsToScore.includes(node.tagName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming of this is modeled on classesToPreserve, which functions as an allowlist for classes that are then not removed from the output (otherwise, all class information is removed).

From that, and from the documentation, and from the ticket you filed ("which can result in the loss of important semantic information and custom function") I would expect this new property to ensure that the content in these custom tags is always preserved. But the patch does not do that, it only scores those elements and they may yet get rejected if their score is too low.

So I think either the patch behaviour needs to be changed (so the tags always stay part of the output, which I suspect is non-trivial to organize), or the documentation/explanation/labeling of the property needs to be changed so that it matches the behaviour. Maybe calling it additionalTagsToScore would be better?

Finally, if you keep the current implementation (but perhaps rename the property), if we need to create a list of scorable tag names let's not do it on every iteration inside this loop, as that is expensive and this loop is hot (performance-sensitive) code.

Copy link
Contributor

@gijsk gijsk left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. See also my note on the ticket you filed; I'm a bit confused about the purpose/rationale of this new property and how to proceed. Let's figure that out before we proceed here. :-)

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.

Preserve custom element tags
2 participants