? ? Pending

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
15 Mar 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

0beb7bf 15 Mar 2020 avatar dgrammatiko init
avatar dgrammatiko dgrammatiko - open - 15 Mar 2020
avatar dgrammatiko dgrammatiko - change - 15 Mar 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 15 Mar 2020
Category Repository Libraries Modules Front End
avatar dgrammatiko dgrammatiko - change - 15 Mar 2020
Labels Added: ? ?
avatar brianteeman
brianteeman - comment - 15 Mar 2020

tested and the accessibility is still ok

avatar joomla-cms-bot joomla-cms-bot - change - 15 Mar 2020
Category Repository Libraries Modules Front End Repository Libraries Modules Front End Templates (site)
605ce93 15 Mar 2020 avatar dgrammatiko CS
avatar laoneo
laoneo - comment - 16 Mar 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.

avatar dgrammatiko
dgrammatiko - comment - 16 Mar 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

avatar wilsonge
wilsonge - comment - 16 Mar 2020

@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')

avatar dgrammatiko
dgrammatiko - comment - 16 Mar 2020

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

84a53ff 16 Mar 2020 avatar dgrammatiko CS
avatar dgrammatiko dgrammatiko - change - 16 Mar 2020
The description was changed
avatar dgrammatiko dgrammatiko - edited - 16 Mar 2020
avatar dgrammatiko dgrammatiko - change - 16 Mar 2020
The description was changed
avatar dgrammatiko dgrammatiko - edited - 16 Mar 2020
avatar laoneo
laoneo - comment - 16 Mar 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.

avatar wilsonge
wilsonge - comment - 16 Mar 2020

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?

avatar laoneo
laoneo - comment - 16 Mar 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]); ?>
avatar dgrammatiko
dgrammatiko - comment - 16 Mar 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.

avatar laoneo
laoneo - comment - 16 Mar 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.

avatar dgrammatiko
dgrammatiko - comment - 16 Mar 2020

@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

avatar laoneo
laoneo - comment - 16 Mar 2020

So let's hear what other do prefer.

avatar wilsonge
wilsonge - comment - 16 Mar 2020

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

avatar dgrammatiko
dgrammatiko - comment - 16 Mar 2020

@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

avatar laoneo
laoneo - comment - 16 Mar 2020

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

avatar dgrammatiko
dgrammatiko - comment - 16 Mar 2020

How can an extension dev load it's own icons?

Do you mean how you can deploy the svgs in the media folder? I think the only prerequisite is that these need to go to /media/vendor/PROVIDER/images/GROUPNAME. So a normal/post installation should cover that.

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',
		// 'group'      => 'solid', // This can be ignored if all the icons are stored in a subfolder named solid
		'icon'       => 'user',
		'classes'    => 'dp-icon dp-icon-user',
		'text'       => Text::_('MOD_LOGIN_VALUE_USERNAME'),
	]
);
avatar brianteeman
brianteeman - comment - 17 Mar 2020

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

avatar wilsonge
wilsonge - comment - 17 Mar 2020

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

avatar brianteeman
brianteeman - comment - 17 Mar 2020

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

avatar wilsonge
wilsonge - comment - 17 Mar 2020

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.

avatar jwaisner jwaisner - change - 17 Mar 2020
Status Pending Discussion
avatar C-Lodder
C-Lodder - comment - 17 Mar 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.

avatar ciar4n
ciar4n - comment - 17 Mar 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?

avatar dgrammatiko
dgrammatiko - comment - 17 Mar 2020

Is this correct?

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

avatar dgrammatiko
dgrammatiko - comment - 17 Mar 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)

avatar ciar4n
ciar4n - comment - 18 Mar 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.

avatar dgrammatiko
dgrammatiko - comment - 18 Mar 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'),
	]
);
avatar ciar4n
ciar4n - comment - 19 Mar 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.

avatar laoneo
laoneo - comment - 19 Mar 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.

avatar dgrammatiko
dgrammatiko - comment - 19 Mar 2020

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)

avatar ciar4n
ciar4n - comment - 19 Mar 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.

avatar dgrammatiko
dgrammatiko - comment - 19 Mar 2020

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.

avatar dgrammatiko
dgrammatiko - comment - 19 Mar 2020

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

avatar dgrammatiko dgrammatiko - close - 19 Mar 2020
avatar dgrammatiko dgrammatiko - change - 19 Mar 2020
Status Discussion Closed
Closed_Date 0000-00-00 00:00:00 2020-03-19 13:15:14
Closed_By dgrammatiko

Add a Comment

Login with GitHub to post a comment