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

Added support for the SvgScript tag #558

Merged
merged 8 commits into from Aug 18, 2019
Merged

Added support for the SvgScript tag #558

merged 8 commits into from Aug 18, 2019

Conversation

gvheertum
Copy link
Member

Reference Issue

Should address #167 which requests the addition of script tags.

What does this implement/fix? Explain your changes.

Allows the creation of SvgScript in the SVG document and can parse the script tags in a SVG file in a type-safe way. Adding of SvgScript was already possible in the past by using a generic tag, having a typed tag for scripts will make handling scripts easier.

Included test cases to test behavior of the script tag on reading and outputting.

[SvgAttribute("type")]
public string ScriptType
{
get { return _scriptType; }
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no special reason, set it to Attributes like any other element.

Copy link
Member Author

Choose a reason for hiding this comment

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

Refactored this. I think I used a wrong example to base this code on. Reworked this to use the attribute collection, this also removed the need to manually output the attributes.

Source/Document Structure/SvgScript.cs Outdated Show resolved Hide resolved
return true;
}

protected override void WriteAttributes(System.Xml.XmlTextWriter writer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to override WriteAttributes ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The code did not correctly output the attributes in the previous setup, so yeah at that moment I did. However using the attributes collection makes this code no longer necessary.

Source/Document Structure/SvgScript.cs Outdated Show resolved Hide resolved
System.XML is now a using instead of a fully qualified class lateron
@gvheertum
Copy link
Member Author

@H1Gdev good remarks, I mad the changes you suggested. Many of the changes were due to me using an incorrect (or outdated) element to base my code on.

Copy link
Member

@mrbean-bremen mrbean-bremen left a comment

Choose a reason for hiding this comment

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

@gvheertum - Thanks a lot, this looks like a helpful extension!
@H1Gdev - thanks for the review! Please check if all findings are addressed - I'll merge after your approval.
As I already wrote, I won't have much free time in the next weeks, so I appreciate someone else reviewing PRs 👍

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.

None yet

3 participants