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

Add a class to empty SVG element when image is not found #66

Closed
wants to merge 0 commits into from

Conversation

meowsus
Copy link

@meowsus meowsus commented May 31, 2017

By adding a class to the empty SVG element, users of this gem will be
able to conditionally style this use case and prevent the browser from
trying to size an empty SVG element on its own.

@meowsus
Copy link
Author

meowsus commented May 31, 2017

I've begun using this helper in my daily development. The only issue I've had with it has been this use case. When an SVG is not found (usually because the image is being loaded dynamically) this gem dumps a loose svg element on the page, containing a useful comment noting which image cannot be found. However, this will frequently break layout.

By adding a simple helper class we can allow users to override the styling of the element in whichever way they please via CSS.

Copy link
Owner

@jamesmartin jamesmartin left a comment

Choose a reason for hiding this comment

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

@meowsus thanks for the pull request. This looks like a solid, rational change to me. I had one question about the CSS class name, but otherwise I think we can ship this in a new release ASAP.

@@ -12,7 +12,7 @@ def inline_svg(filename, transform_params={})
configured_asset_file.named filename
end
rescue InlineSvg::AssetFile::FileNotFound
return "<svg><!-- SVG file not found: '#{filename}' #{extension_hint(filename)}--></svg>".html_safe
return "<svg class='svg-not-found'><!-- SVG file not found: '#{filename}' #{extension_hint(filename)}--></svg>".html_safe
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if this CSS class should be something more specific to avoid potential collisions with existing user-defined classes. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

My first thought was to make this configurable, but then I saw that this gem doesn't have configuration already. Maybe that might make sense to add as well? It would certainly help out the efforts of #48

Copy link
Owner

Choose a reason for hiding this comment

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

We could add this configuration to the InlineSvg::Configuration class. Is that the kind of configuration you were thinking?

We could use it like this:

InlineSvg.configure do |config|
  config.svg_not_found_css_class = "my-custom-class"
end

@gerrywastaken
Copy link

@meowsus Out of curiosity, what was the resolution to this?

@meowsus
Copy link
Author

meowsus commented Jul 26, 2017

@gerrywastaken It was merged into the latest patch in a different PR. Sorry for the confusion.

#67

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants