Skip to content

Comments

[4.0] [RFC] SVG icons#28351

Closed
dgrammatiko wants to merge 11 commits intojoomla:4.0-devfrom
dgrammatiko:svg_icons
Closed

[4.0] [RFC] SVG icons#28351
dgrammatiko wants to merge 11 commits intojoomla:4.0-devfrom
dgrammatiko:svg_icons

Conversation

@dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Mar 15, 2020

Pull Request for Issue #28262 .

Summary of Changes

Introduce a new API for inline svg icons.
Usage:

<?php
echo Factory::getDocument()->setIcon(
	[
		'provider'   => 'fontawesome-free',
		'group'      => 'solid',
		'icon'       => 'user',
		'classes'    => 'demo-class',
		'text'       => Text::_('MOD_LOGIN_VALUE_USERNAME'),
		'attributes' => [
			'data-foo' => 'bar',
			'id'       => 'foobar'
		]
	]
);
?>

Why?

Just google svg vs font icons (here is the first result: https://www.keycdn.com/blog/icon-fonts-vs-svgs , but there are more articles all pointing out that svg's are the preferred way nowadays)

Why not an implementation with Layouts?

Read my comment here: #28262 (comment)
Also to add one more thing this API is just extending the already existing API for images: HTMLHelper::image()

Testing Instructions

Apply the Patch, run npm ci
Check the landing page on the Cassiopeia.
You should get this:
Screenshot 2020-03-15 at 15 39 09

Documentation Changes Required

Yes the Api needs documentation, eg usage cases and also 3rd party issuing svg packs

@ciar4n @wilsonge @laoneo

@joomla-cms-bot joomla-cms-bot added PR-4.0-dev RFC Request for Comment labels Mar 15, 2020
stroke-width: 16px;
}
CSS
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ciar4n I've added this css for demo purposes I guess you will come up with something better in the template level

Copy link
Contributor

@ciar4n ciar4n Mar 16, 2020

Choose a reason for hiding this comment

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

The majority of SVG icons would be displayed inline so might be an idea to apply the styling for that by default...

svg {
  display: inline-block;
  font-size: inherit;
  max-height: 1em;
}

@brianteeman
Copy link
Contributor

tested and the accessibility is still ok

@laoneo
Copy link
Member

laoneo commented Mar 16, 2020

I do not understand why you are not using a layout file for this. It is much more convenient for site admins as rendering of output should be overridable. Now it is hardcoded somewhere in the HTMLDocument class.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Mar 16, 2020

I do not understand why you are not using a layout file for this

A layout for a pretty well defined element (<svg />) ? What will be the benefit? The attributes are pretty limited and well defined. Joomla is doing this for

public static function iframe($url, $name, $attribs = null, $noFrames = '')
,
public static function link($url, $text, $attribs = null)
and a bunch more tags.

To be clear here I'm not against it but there should be some actual reason that a static asset should be treated as a layout, maybe I have narrow view on this so if you could elaborate on the benefits will be helpful

@wilsonge
Copy link
Contributor

@laoneo so the advantage of using this way is that you're only including the SVG once in the DOM and the inline SVG's are then referencing the single include. This means for commonly used layouts your reducing the page load size. Templates still have the ability to override as the SVGs run through HTMLHelper::_('image')

@dgrammatiko
Copy link
Contributor Author

So I've added the ability to push any kind of attribute to the svg element. The example in the user icon now looks like:
Screenshot 2020-03-16 at 12 33 39
We're just echoing an svg with a child use element there that is referencing the actual svg which is just the inlined static svg (enhanced with the appropriate attributes for a11y: role + title) that is echoed at the bottom of the document (or in any place the template implementor thinks is more appropriate):
Screenshot 2020-03-16 at 12 33 52

}

$attribs = '';
$provider = $icon['provider'] ? $icon['provider'] : 'fontawesome-free';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the option to state a provider? Is this really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we want 3pd to roll their own icon sets. Hardcoding this to Font Awesome (by the way it's the fallback, meaning for the core you could ignore this or the group option below, if the icon is in the group solid) is rather limiting

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm missing something but most custom SVGs would not require any of the following...

'provider'   => 'fontawesome-free',
'group'      => 'solid',
'icon'       => 'user',

Even with FA these are mostly covered by the class option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These options actually resolve the path to the icon... The icons are not stored in the same directory

Copy link
Contributor

Choose a reason for hiding this comment

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

Does that mean that these are required?

@laoneo
Copy link
Member

laoneo commented Mar 16, 2020

For me this is more a strategic question and it goes into the same direction as you did with the form fields to put all of them into layouts. For now SVG's are state of the art, nobody knows what comes next, so it would be nice when there is a way for template devs to override the creation of icons.

In my next DPCalendar release I use the reuse possibility as well. So the first icon is rendered normally and all subsequent ones do use it as reference. With the current pr you will have two SVG's even when only one icon is rendered.

I don't want to make a big deal out of it, but this pr looks over-engineered for me.

@wilsonge
Copy link
Contributor

In my next DPCalendar release I use the reuse possibility as well. So the first icon is rendered normally and all subsequent ones do use it as reference. With the current pr you will have two SVG's even when only one icon is rendered.

But where are you tracking which icons are in use?

@laoneo
Copy link
Member

laoneo commented Mar 16, 2020

I have a cache variable, basically my layout (block/icon.php) looks like:

$icon = basename($displayData['icon']);
$path = JPATH_ROOT . '/templates/' . JFactory::getApplication()->getTemplate() . '/images/com_dpcalendar/icons/' . $icon . '.svg';
if (!file_exists($path)) {
	$path = JPATH_ROOT . '/templates/' . JFactory::getApplication()->getTemplate() . '/images/icons/' . $icon . '.svg';
}
if (!file_exists($path)) {
	$path = JPATH_ROOT . '/media/com_dpcalendar/images/icons/' . $icon . '.svg';
}
if (!file_exists($path)) {
	return '';
}

if (in_array($path, \DPCalendar\HTML\Block\Icon::$pathCache)) {
	$content = '<svg width="10" height="10"><use href="#dp-icon-' . $icon . '"/></svg>';
} else {
	\DPCalendar\HTML\Block\Icon::$pathCache[] = $path;

	$content = @file_get_contents($path);
	if (!empty($displayData['title'])) {
		$content = str_replace('><path', '><title>' . $displayData['title'] . '</title><path', $content);
	}

	$content = str_replace('<svg', '<svg id="dp-icon-' . $icon . '"', $content);
}
?>
<span class="dp-icon dp-icon_<?php echo $icon; ?>"><?php echo $content; ?></span>

It can be called then like:

<?php echo JLayoutHelper::render('block.icon', ['icon' => \DPCalendar\HTML\Block\Icon::PRINTING]); ?>

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Mar 16, 2020

['icon' => \DPCalendar\HTML\Block\Icon::PRINTING]

This part will not work with Font awesome (unless if we flatten the folders to a single folder)
Also will (probably) disallow the usage of dualtone (or any future type of icons) without major redesign

PS Probably we can have multiple layouts, eg: solid, dualtone, etc to overcome this.
But still the output <span class="dp-icon dp-icon_<?php echo $icon; ?>"><?php echo $content; ?></span> is not any different than this implementation. Also in this approach you could have different sized icons (through a class) in the layout approach someone needs to create a specific layout for that
Also the approach that the first icon is the actual svg and then every iteration is just a reference to that is the optimal option for reduced size. I'll have to acknowledge that with the layout approach you have 0 wasted bytes.

@laoneo
Copy link
Member

laoneo commented Mar 16, 2020

This are technical limitations. From an extension developer perspective all I want is to render an icon with an identifier. I do not want to care where the icon is.

@dgrammatiko
Copy link
Contributor Author

@laoneo to be totally fair here I really don't care if it's my approach or the layouts. What is needed is some way so frontenders could reference an icon and that will be injected in the output. This PR was created as RFC as I don't think that I have all the answers to every problem, so these kind of arguments are really constructive imho. As long as there is a way so I don't have to copy paste the source of an icon to the layouts I'm fine, Layouts, HTMLHelper (didn't;t go that way because there is already an icon) or document is minor here

@laoneo
Copy link
Member

laoneo commented Mar 16, 2020

So let's hear what other do prefer.

@wilsonge
Copy link
Contributor

I need you guys to come up with a proposal. Like this is one of those things where it needs to be template + extension devs coming together coming up with a proposal you can agree on rather than me dictating less popular things down to you

@dgrammatiko
Copy link
Contributor Author

@wilsonge then invite template creators and extensions devs (with front end facing code) here. It shouldn't be an argument between few people, expand the base to get a better answer

@laoneo
Copy link
Member

laoneo commented Mar 16, 2020

Ok. There is also one thing missing here. How can an extension dev load it's own icons?

@brianteeman
Copy link
Contributor

Do I understand correctly that this new method for the icons will be applied throughout core BUT the current method will still be available for extensions if they wish. ie there is no unnecessary b/c/ break

@wilsonge
Copy link
Contributor

Well the current method is just rendering html - which we couldn't stop people using even if they wanted to

@brianteeman
Copy link
Contributor

not about stopping them its about supporting them. ie we keep the css and the import of the fonts

@wilsonge
Copy link
Contributor

https://github.com/joomla/joomla-cms/blob/4.0-dev/templates/cassiopeia/joomla.asset.json#L13

We'd drop font awesome (frontend template only) from the asset list loaded by default in cassiopeia. We'd still have the files shipped so people could import the CSS files with a one line of PHP to import via web asset manager.

@C-Lodder
Copy link
Member

C-Lodder commented Mar 17, 2020

not about stopping them its about supporting them. ie we keep the css and the import of the fonts

as long as the font and CSS are loaded as a dependency for the template in the asset manager and not in Joomla's core then I'm happy.


As a template dev, this will make life so much easier. A nice way to decouple and be able to use your own fonts.

In in 2 minds about both approaches (this vs layouts). The only problem I see with this is if you wanted to change the font from one to another, you'd (like everything else these days) have to override the core views, which if you've ever developed a template from scratch, would know can be an absolute pain. Whereas overriding a single layout directory would be much easier to maintain.

@ciar4n
Copy link
Contributor

ciar4n commented Mar 17, 2020

A common enough use case here, I imagine, would be overriding the icons used in a view without having to override the entire view. If I understand this PR correctly, that would mean having a ../media/icons/font-awesome folder in the template and swapping out the font awesome icons for the alternative SVGs. Is this correct? And if so, is it not strange to have non FA icons in a FA folder?

@dgrammatiko
Copy link
Contributor Author

Is this correct?

Yes that's the intention here, leave everything else intact and just swap the svg file

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Mar 17, 2020

And if so, is it not strange to have non FA icons in a FA folder?

No, There might be extension devs that want to roll their own icons, you still can override them in the same way swapping the files in the template/images/etc...
It's all about helping both the template devs and the extensions dev. I get that then a template might need to provide special overrides (svg files) for some extensions but it's not any different than the rest overrides atm (or at least this is my perception here)

Co-Authored-By: Brian Teeman <brian@teeman.net>
@ciar4n
Copy link
Contributor

ciar4n commented Mar 18, 2020

There might be extension devs that want to roll their own icons,

I realise that. I'm just not sure SVGs need to be coupled to an icon set.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Mar 18, 2020

@ciar4n they are not coupled to any set, check the same example as the description with an imaginary implementation from an extension dev (digital-peak is the dev's company name) :

eg: /media/vendor/digital-peak/images/solid (the solid subfolder is the default one)

Then use it like:

echo Factory::getDocument()->setIcon(
	[
		'provider'   => 'digital-peak',
		'icon'       => 'user',
		'classes'    => 'dp-icon dp-icon-user',
		'text'       => Text::_('MOD_LOGIN_VALUE_USERNAME'),
	]
);

@ciar4n
Copy link
Contributor

ciar4n commented Mar 19, 2020

I guess what I mean is that in the core views, icons are coupled with Font Awesome.

Consider maybe removing the 'group' option. Font Awesome is the only icon set I know of that has multiple groups in the same family. If 'groups' are defined by the weight of the icon which is largely the case in FA then it is unlikely you would want to load icons of different groups. Telling devs that icons should be placed in a 'solid' folder if no group is defined is a little odd.

@laoneo
Copy link
Member

laoneo commented Mar 19, 2020

Agree here with the argument of @ciar4n. Honestly as extension dev I do not care what for a provider is used. All I want is that the icon I need is rendered. So first it should look in the template if such an icon exists, if not then it should look in core and as fallback in the component. So I would replace provider with component. Means the component needs to deliver the icon in the installation package when it doesn't exist in core.
Please no vendor lock in by forcing provider specific settings.

@dgrammatiko
Copy link
Contributor Author

Please no vendor lock in by forcing provider specific settings.

My approach is not any different from the current approach of Joomla for the rest of the assets (js, css) the only difference (for shake of simplicity of the code) is that the icons need to go to vendor/Provider, now provider can be a component/module/plugin (eg dpcalendar) it doesn't have to be the company name (although it's totally reasonable, because a vendor might reuse the same path).

BUT I think you misunderstand the concept here. The folder structure has nothing to do with vendor lock in. I will suggest you to download the fontawesome svgs and inspect the folder structure (it worth also d/l the pro version) because the priority here is to serve correctly FA, then 3rd PD and do that in a way that template creators are able to switch JUST the svg file. (this is already achieved by this piece of code)

@ciar4n
Copy link
Contributor

ciar4n commented Mar 19, 2020

I think this PR primarily provides a really good API to serve Font Awesome in a performant manner that is much improved on the current setup. There a small cost there when it comes to overriding. I appreciate an icon font is very much an asset and should be treated as such, I am not sure I can say the same for individual SVGs. I do like the idea of a user been able to paste some SVG code into an override in Joomlas template editor. Something not really possible here.

I am still in two minds about the best solution. Both work for me but still important to consider all possibilities. There is no solution with all the answers.

@dgrammatiko
Copy link
Contributor Author

do like the idea of a user been able to paste some SVG code into an override in Joomlas template editor.

That will be very hard to be a11y for multilingual sites, unless if we fall back to we're accessible by hiding things we cannot properly describe to the visually impaired users..., which basically is what the whole backend is doing.

@dgrammatiko
Copy link
Contributor Author

After quite some though I think that Joomla will be better with the Layout approach therefore I'm closing this

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

Labels

RFC Request for Comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants