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

Even further with Tag... #36

Open
wants to merge 148 commits into
base: main
Choose a base branch
from
Open

Conversation

bgisme
Copy link
Contributor

@bgisme bgisme commented Jul 12, 2023

This PR goes further than my previous one with changes to Tag

And I'll admit it may be a bridge too far. Not sure. But I thought I'd put it out there.

It tries to address some things that have always struck me as odd or confusing about the library...

Tag appears like the base class. And then GroupTag is a derivation. But it should really be the other way around. GroupTag is more foundational. It's just a renderless collection of child Tags. Every other kind of Tag is a specialization: name, attributes and even more children. So this PR eliminates GroupTag and makes Tag renderless by default. And then provides options and subclasses to specialize. TagBuilder just puts components inside another Tag.

Node has always confused me. And why its properties should be separate. Maybe there's a good reason, but I don't see it in the library. So I moved them all into Tag. And I think it makes how to use the class more apparent. I also eliminated .group and made the type property on Tag optional. Because that's essentially what the class is... a nothing, invisible container.

• Lots of empty arrays get allocated. Tag.children and Node.attributes are[] by default. And while it might not seem like a lot memory allocation, I wonder if it adds up in large hierarchies. So I made them optional.

@bgisme
Copy link
Contributor Author

bgisme commented Jul 27, 2023

Not sure if you started working on the merge yet.

But I added a few more things.

Like handling the "lang" attribute

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

Successfully merging this pull request may close these issues.

2 participants