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

Merge xilem_html and xilem_svg as xilem_web #142

Merged
merged 7 commits into from
Jan 4, 2024

Conversation

Philipp-M
Copy link
Contributor

@Philipp-M Philipp-M commented Nov 4, 2023

Merge these libraries together, as they share a lot of code. xilem_svg is completely merged/ported with xilem_html as xilem_web (name is open for discussion).

The views in xilem_svg were either removed (Clicked or Group as they're redundant because of OnClick or G (SVG g element)), refactored (add generic parameters, for better generic inference), and/or generalized (e.g. Pointer which can now be used for all Elements, or Fill/Stroke which can also be used for raw SVG elements such as <circle> or <text>).

This kind of introduces the idea, that a fully-typed close(r) to the spec following HTML/DOM API can coexist with custom views/widgets (in this case kurbo shapes used as SVG basic shapes, previously in xilem_svg)

@Philipp-M Philipp-M force-pushed the merge-xilem_html-and-xilem_svg branch 2 times, most recently from c92231d to 35f0910 Compare November 7, 2023 20:46
@Philipp-M Philipp-M force-pushed the merge-xilem_html-and-xilem_svg branch 2 times, most recently from 859e712 to 199fee8 Compare November 25, 2023 10:40
@Philipp-M Philipp-M force-pushed the merge-xilem_html-and-xilem_svg branch from 199fee8 to 0183814 Compare November 28, 2023 19:33
@Philipp-M Philipp-M marked this pull request as ready for review November 28, 2023 19:35
@richard-uk1
Copy link
Contributor

I'm keen for this to happen - I will try to find time for review.

@raphlinus
Copy link
Contributor

Extremely awesome! I was able to get my beztoy example ported over with minimal fuss, and I'm looking forward to being able to wire up HTML sliders (I haven't done this yet, but expect it to be straightforward).

The one snag I hit is that I had put a .fill(NONE) on a g to disable fills of all the paths in the group, but that no longer seems to be enabled. This was not hard to work around (just set that on each of the paths), but feels like it should work.

I haven't reviewed the code in detail (only checked to see whether it works) but based on a skim I don't have any major concerns. I could do a deeper review if that would be helpful.

@Philipp-M
Copy link
Contributor Author

Thanks :)

Well, I guess I need to dig a little bit deeper what is allowed in SVG and what not (my experience with SVG is limited yet...). g seems to inherit all kinds attributes for its children etc. so I've added stroke and fill to the SvggElement interface/trait in the latest commit, such that they're now generally available on the g element. I haven't found NONE though, I guess Color::TRANSPARENT has the same effect though (and it should behave the same as fill in xilem_svg).

(And I renamed Cx::add_new_attribute_to_current_element -> Cx::add_attr_to_element and IntoAttributeValue::into_attribute_value -> IntoAttributeValue::into_attr_value since it's less verbose, I don't think that's worth its own PR...)

I feel rather confident about the code, so a deeper review may be helpful but I don't think it's really necessary.

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

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

I think this is ready to merge, as the latest change addresses my concern. Thanks!

@Philipp-M Philipp-M changed the title Merge xilem_html and xilem_svg Merge xilem_html and xilem_svg as xilem_web Jan 4, 2024
@Philipp-M Philipp-M added this pull request to the merge queue Jan 4, 2024
Merged via the queue into linebender:main with commit ea45b9f Jan 4, 2024
7 checks passed
@Philipp-M Philipp-M deleted the merge-xilem_html-and-xilem_svg branch March 13, 2024 22:23
DJMcNab added a commit to DJMcNab/xilem that referenced this pull request Jul 2, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jul 2, 2024
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