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

Font embedding changes #38

Merged
merged 16 commits into from
Sep 5, 2013
Merged

Font embedding changes #38

merged 16 commits into from
Sep 5, 2013

Conversation

jbracker
Copy link
Member

@jbracker jbracker commented Sep 4, 2013

This pull request already contains all bug fixed from master in addition to the changes needed to embed fonts in SVGs.

Basically I only added a optional SVG defs section that can be filled with user content (e.g. font data).

Jan Bracker added 14 commits August 29, 2013 12:07
Before this several stacked clips were just printed as several paths in the clipPath element.
But according to the SVG standard this means they are OR'ed together, although we want them to be intersected.
I changed this by building them up around the clipped element hierarchical, which leads to intersection according to the SVG spec.
I tested this in the chart drawing tests and it seems to work.
}
deriving Show
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove this Show instance? Is S.Svg not an instance of Show? The Show instance is needed by diagrams-builder to include backend options in the hash of a diagram (so that the diagram will be recompiled if the options change).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I removed it because S.Svg is not an instance of Show. I wanted to replace it but I wasn't sure what would be appropriate. Should I output the XML?

Copy link
Member

Choose a reason for hiding this comment

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

OK, I see that Svg = Markup = MarkupM () is indeed not an instance of Show, so we can no longer derive a Show instance. We could implement a custom Show instance which simply serializes the SVG. Probably that's what we should do for now, to avoid breaking diagrams-builder at the moment. But this is not really in the spirit of Show. Probably what we should actually do is switch to using Hashable instead of Show in diagrams-builder. Filed diagrams/diagrams-builder#5.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then I will just insert the SVG as a XML string and provide a custom instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

See 86a2688 and f434e40

@byorgey
Copy link
Member

byorgey commented Sep 5, 2013

Thanks!

byorgey added a commit that referenced this pull request Sep 5, 2013
@byorgey byorgey merged commit d977cda into diagrams:master Sep 5, 2013
@jbracker jbracker deleted the font-embedding branch September 5, 2013 13:15
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