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

Icon #969

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Icon #969

wants to merge 2 commits into from

Conversation

ebellempire
Copy link
Contributor

@ebellempire ebellempire commented Oct 13, 2021

This adds a simple icon() function, three example icons (from @ionic-team/ionicons), and a README file containing rationale, usage examples, and a few other considerations.

#967

@ebellempire
Copy link
Contributor Author

@todo

  • choose and add the icon set
  • add to core/README ("Omeka includes...")
  • admin theme support? (CSS)
  • documentation

Potential icon sets:

Of these sets, Ionicons and Bootstrap would probably be the easiest to add/maintain as submodules since they have a simpler directory structure.

Example Usage and Details

Some sets, such as Ionicons, use a dash-suffix for variants (e.g. sharp, outline). Depending on the chosen set, the $variants param may not be necessary but it could still be useful. Consider the utility of, say, a theme configuration for changing all icon variants at once.

<?php
// default usage...
echo icon('accessibility');

// 'sharp' variant...
echo icon('accessibility', 'sharp');

// custom 'omeka' icon in theme 'custom_icons' directory...
echo icon('omeka', null, 'images/custom_icons');

// setting the icon variant using a theme option...
$variant = get_theme_option('icon_style');
echo icon('headset', $variant);
echo icon('folder', $variant);
echo icon('globe', $variant);
?>

Example HTML Output

Unlike SVGs displayed via an img tag, inline SVG can be styled with CSS.

<span class="icon globe outline">
	<svg height="512" viewBox="0 0 512 512" width="512" xmlns="http://www.w3.org/2000/svg">
	[...] </svg>
</span>

Default CSS

For most themes (including admin), this would probably be a sufficient base...

.icon svg {
	height: 1.5em;
	width: 1.5em;
	vertical-align: middle;
	fill: currentColor;
	transition: 0.25s fill linear;
}
.icon svg path,
.icon svg circle {
	stroke: currentColor;
	transition: 0.25s stroke linear;
}

@zerocrates
Copy link
Member

zerocrates commented Nov 3, 2021

I'm wondering about a couple things here:

  • Do we want to call this something other than just "icon", seeing as we already have legacy icons of another system and could have yet another in the future. Should this more specifically say that it's doing SVG?
  • Do we want to do something more like SVG "sprites"? I'm a little concerned about the markup size increase from making every instance of an icon the full SVG inline if we're using it several times in a single page. And we'd also be doing a lot of opening of files to pull in that markup in the case of many icons and/or many usages.
  • Down more in the weeds, I don't think the "hyphenated variant" pattern is enough of "a thing" that it justifies its own baked-in part of the signature, vs. just asking for whatever the full name is in a single string. (As to your point about a theme using the variant parameter to change everything at once, I imagine in a case like that where the theme knows its actually dealing with a set that works that way then it can just lightly wrap over the core function to provide that if it were desired)
  • The README and example icon stuff will eventually have to come out of the PR if we're going to merge it. Though I appreciate that the icons in particular make it easier to "test-drive."

@ebellempire
Copy link
Contributor Author

ebellempire commented Nov 5, 2021

Function name

I could see changing the function name if that's something that makes sense on your end.

Performance

As far as markup size, I had the same thought until I compared inline SVG to the cost of Font Awesome. For instance, I have a page that uses 16 SVG icons (navigation stuff, social icons, etc). They add up to about 8.5Kb. The FA WOFF included in Omeka admin is about 75Kb, plus a little more for the required CSS. Then there's the cost of the multiple network requests for FA. I don't know how that compares to the cost of reading a bunch of SVGs but without doing a lot of server-side tests, I'd venture that all things considered it's probably faster to read the files than to fetch/embed the font. But I could be wrong. So far, I've just been trying to reduce network requests and optimize the overall size of the delivered page.

Variant

Ultimately, not a big deal either way since, as you note, it's easy enough to accomplish the desired outcome without it. Will remove, along with the README file and example icons.

ebellempire added a commit to ebellempire/Omeka that referenced this pull request Nov 5, 2021
@zerocrates
Copy link
Member

The issue on transfer size is that you pay that cost once per user with the webfont, then it's cached, while the increased page size is there for every request in the inline-SVG world. So I'm not quite sure how to best compare that. Using the SVGs just as regular images seems to forfeit the benefits of being able to CSS-style them and so on, and yes would result in additional round-trips for each unique icon at least once, which is probably not great.

The "sprite" solution avoids both of these by not duplicating the icon markup when it's used multiple times within a page but also not incurring a bunch of extra requests... but it's more complicated to do. I think you can just load an external SVG and still use it as "sprites" and keep the styling benefits, though I imagine that gets you back into a similar transfer-size space as the webfont did originally.

A hybrid approach where this function notices you using an icon more than once and kind of "sprites" it for you could be the best of both worlds, so rather than outputting the full markup every time, the icon markup only is output once and later instances of it (or all instances of it) are <use> references.

@ebellempire
Copy link
Contributor Author

Agreed, some solution that allows for caching would be ideal. I'll look into this some more and see if there's something I can contribute that strikes the right balance. Feel free to close this PR if you like, or I can come back to it later.

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

2 participants