Skip to content

SVGNode Attribute Getters and Setters#70

Open
wilgaboury wants to merge 22 commits into
HumbleUI:masterfrom
wilgaboury:svg-node-attributes
Open

SVGNode Attribute Getters and Setters#70
wilgaboury wants to merge 22 commits into
HumbleUI:masterfrom
wilgaboury:svg-node-attributes

Conversation

@wilgaboury

Copy link
Copy Markdown

Bindings for the assorted attribute getters and setters on SkSVGNode.

@wilgaboury

Copy link
Copy Markdown
Author

Wanted to put this pull request up for some initial review and feedback. I'm also planning to give it a thorough self-review this week and will likely make some small changes.

@tonsky

tonsky commented Sep 17, 2024

Copy link
Copy Markdown
Contributor

Sorry, a bit busy, will look at it next week

@wilgaboury

Copy link
Copy Markdown
Author

No worries! I know it's a big change to submit all at once, so please don't feel like you have to rush to review it.

@tonsky

tonsky commented Sep 23, 2024

Copy link
Copy Markdown
Contributor

Looks great to me, would be happy to have it. How about having a Scene demonstrating some of these new APIs? Maybe not all, but just a few?

@wilgaboury

Copy link
Copy Markdown
Author

Sounds good, I'll add a scene later this week.

@tonsky

tonsky commented Sep 24, 2024

Copy link
Copy Markdown
Contributor

From quick overview, I would love to see:

  • Use of currentColor, and passing it from outer world (e.g. SVG that uses currentColor, and we change it on hover from the scene)
  • Modifying some attribute inside SVG dynamically (also on hover?)

@wilgaboury

Copy link
Copy Markdown
Author

Right now, I don't think it's possible to pass in currentColor from the outer world. I would have to add additional bindings for SkSVGRenderContext and the specific method SkSVGDOM.renderNode (which I would like to do but is going to take more time).

That being said I've messed around with dynamically changing the root nodes fill color (which is inherited by the rest of the SVG) and making a scene that changes it on hover should be easy.

@tonsky

tonsky commented Sep 25, 2024

Copy link
Copy Markdown
Contributor

Yeah let’s do that. The idea is to demonstrate/test what is available

@SonarSonic

Copy link
Copy Markdown
Contributor

This would be really great addition to me, as someone using Skiaj for rendering SVGs. Let me know if there's anything I can do to help get the PR merged. Happy to try create the test scene for it if you don't have time @wilgaboury

@wilgaboury

Copy link
Copy Markdown
Author

Sorry about the long delay. I've been pretty busy lately and somewhat forgot about this PR. Creating the scene should be fairly easy and I'll try to get it done soon.

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.

3 participants