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.

Sign up for GitHub

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

Add macro to inline SVG files #1761

Merged
merged 6 commits into from
Nov 17, 2022
Merged

Add macro to inline SVG files #1761

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
33042d3
Add macro to inline SVG files
wout Nov 15, 2022
15cc6fa
Add modifier for originally styled attributes
wout Nov 15, 2022
6220cad
Add test for inlined svg with its original styling
wout Nov 15, 2022
a50e31d
Create separate test cases for stripping out unwanted elements in SVGs
wout Nov 17, 2022
a59d0b4
Replace newlines with a space to avoid attribute concatenation
wout Nov 17, 2022
c107760
Strip leading an trailing whitespace on icons
wout Nov 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions spec/fixtures/lucky_logo.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
37 changes: 37 additions & 0 deletions spec/lucky/text_helpers/svg_inliner_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
require "./text_helpers_spec"

@[Lucky::SvgInliner::Path("spec/fixtures")]
module Lucky::SvgInliner
end

class TextHelperTestPage
def with_inlined_svg
inline_svg("lucky_logo.svg")
end

def with_inlined_and_styled_svg
inline_svg("lucky_logo.svg", false)
end
end

describe Lucky::SvgInliner do
describe ".inline_svg" do
it "inlines an svg in a page" do
inlined_svg = view.tap(&.with_inlined_svg).render
inlined_svg.should start_with %(<svg data-inline-svg="lucky_logo.svg")
inlined_svg.should_not contain %(<?xml version="1.0" encoding="UTF-8"?>)
inlined_svg.should_not contain %(<!-- lucky logo -->)
inlined_svg.should_not contain "\n"
inlined_svg.should_not contain %(fill="none" stroke="#2a2a2a" class="logo")
end

it "inlines an svg in a page with its original styling attributes" do
inlined_svg = view.tap(&.with_inlined_and_styled_svg).render
inlined_svg.should start_with %(<svg data-inline-svg-styled="lucky_logo.svg")
inlined_svg.should contain %(fill="none" stroke="#2a2a2a" class="logo")
inlined_svg.should_not contain %(<?xml version="1.0" encoding="UTF-8"?>)
inlined_svg.should_not contain %(<!-- lucky logo -->)
inlined_svg.should_not contain "\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that this strips out un-needed stuff from the XML. Should these should_not cases be broken out into an it "strips out comments" and it "strips out the xml header" set of tests? As it is, if something comes up and someone needs to modify the filter, they don't have a test which directly correlates to that feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll add separate specs for those cases separately.

end
end
end
1 change: 1 addition & 0 deletions src/lucky/html_builder.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ module Lucky::HTMLBuilder
include Lucky::RenderIfDefined
include Lucky::TagDefaults
include Lucky::LiveReloadTag
include Lucky::SvgInliner

abstract def view : IO

Expand Down
27 changes: 27 additions & 0 deletions src/lucky/page_helpers/svg_inliner.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
annotation Lucky::SvgInliner::Path
end
annotation Lucky::SvgInliner::StripRegex
end

@[Lucky::SvgInliner::Path("src/svgs")]
@[Lucky::SvgInliner::StripRegex(/(class|fill|stroke|stroke-width|style)="[^"]+" ?/)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me understand what's going on with these annotations? It seems like a roundabout way to accomplish arguments to the macro.

Copy link
Contributor Author

@wout wout Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sure. I think having those parameters passed as arguments would be tedious. Let's say the user wants to use a different directory for the SVG files, then it suffices to override the annotation in an initializer:

# config/svg_inliner.cr
@[Lucky::SvgInliner::Path("src/icons")]
module Lucky::SvgInliner
end

As opposed to (for every single SVG):

inline_svg("menu-social/twitter.svg", false, "src/icons")
inline_svg("menu-social/discord.svg", false, "src/icons")

Does that answer your question? Or am I missing the point?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting, I hadn't considered that. I'm not opposed. Another possibility for this would be to have a Habitat config parameter. @jwoertink @stephendolan thoughts?

Copy link
Contributor Author

@wout wout Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Tried that first, but settings from Habitat are not available at compile-time. As far as I know, the only way to allow compile-time configuration is by using annotations or constants. And using constants is not favourable because it's impossible to override them.

I think these are sensible defaults, so in 99% of the cases, it wouldn't be necessary to change them. The annotations are just an escape hatch.

Copy link
Member

Choose a reason for hiding this comment

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

It's different for sure, but I'm willing to give it a shot. I don't have a strong opinion on it vs just using Habitat.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wout yeah, you present a use case I can't argue with. Thank you for walking me through it. Let's go!

module Lucky::SvgInliner
macro inline_svg(path, strip_styling = true)
{%
svgs_path = Lucky::SvgInliner.annotation(Lucky::SvgInliner::Path).args.first
regex = Lucky::SvgInliner.annotation(Lucky::SvgInliner::StripRegex).args.first
full_path = "#{svgs_path.id}/#{path.id}"

raise "SVG file #{full_path.id} is missing" unless file_exists?(full_path)

svg = read_file(full_path)
.gsub(/<\?xml[^>]+>/, "")
.gsub(/<!--[^>]+>/, "")
.gsub(/\n\s*/, "")
svg = svg.gsub(regex, "") if strip_styling
modifier = strip_styling ? "" : "-styled"
%}

raw {{svg.gsub(/<svg/, %(<svg data-inline-svg#{modifier.id}="#{path.id}"))}}
end
end