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

Use inline icons as components #2670

Merged
merged 1 commit into from May 19, 2022
Merged

Use inline icons as components #2670

merged 1 commit into from May 19, 2022

Conversation

jcoyne
Copy link
Member

@jcoyne jcoyne commented Mar 17, 2022

This makes the icons simple to customize and allows us to deprecate behavior that deals with the internals of the asset pipeline. This approach is an alternative to #2668 where we add awareness of other asset pipelines (e.g. propshaft).

@cbeer
Copy link
Member

cbeer commented Mar 17, 2022

We've had a lot of luck recently just putting the SVG inline in the CSS like bootstrap's now doing:

https://github.com/twbs/bootstrap/blob/main/scss/_variables.scss#L881

@jcoyne
Copy link
Member Author

jcoyne commented Mar 17, 2022

@cbeer the problem with that approach is you can't style the svg using css, can you? For example fill="currentColor" only works for inline svg. I think this is why we've been in-lining these icons in this way. This is also the approach taken by bootstrap icons. See the example in https://icons.getbootstrap.com/icons/search/

@jcoyne jcoyne force-pushed the inline-icons branch 3 times, most recently from 7584a17 to 49057c7 Compare March 18, 2022 20:36
Copy link
Member

@tpendragon tpendragon left a comment

Choose a reason for hiding this comment

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

We talked about this on the committer's call and like this pattern. We'd just like to see documentation in the class or method level to describe how to customize the icon if that's what you want to do

@jcoyne jcoyne force-pushed the inline-icons branch 3 times, most recently from deedcca to 60536ba Compare May 18, 2022 18:54
@jcoyne
Copy link
Member Author

jcoyne commented May 18, 2022

@tpendragon I added class documentation in the two icon classes.

@carolyncole
Copy link
Contributor

Another option I had to research is to still read the svg out of a file...
https://www.seancdavis.com/posts/render-inline-svg-rails-middleman/
I'm not suggesting this PR needs to change, just putting this documentation out there if we regret putting the SVG in rails code.

@tpendragon
Copy link
Member

CI's failing, but besides that 👍

This makes the icons simple to customize and allows us to deprecate behavior that deals with the internals of the asset pipeline
@barmintor
Copy link
Contributor

barmintor commented May 19, 2022

I'm thinking about @hackartisan's questions yesterday - if we made the new components simple extensions of Blacklight::BaseComponent, could the svg source be a sidecar/template file? That would keep the svg out of the ruby itself. Not sure if svg is one of the ActionView template handlers, but that would be super easy to test.

Edit: I could help look into that post-merge, but it's a separate change.

@barmintor barmintor merged commit 673559a into main May 19, 2022
@barmintor barmintor deleted the inline-icons branch May 19, 2022 13:15
jcoyne added a commit that referenced this pull request May 19, 2022
This is a backport of #2670
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

5 participants