This looks pointless and counter-intuitive.

Can you please remove $iconPrefix = 'icon-', $iconSuffix = null, $html = true, $elem = 'span',

• We shoudn't support custom prefixes (make no sense)
• Don't know why the suffix should be an extra parameter, also php should not be used for styling
• html parameter makes no sense if you supply the complete icon "class" (without icon-)
• $elem should not be provided by php because it's a design/template decision, if I override the icon function because I want to use svg I have a problem if someone provides an unexpected markup.
• Children, don't know if this is a good idea, this would limit us by changing the iconmark to another format (which maybe can't support child elements)

@hans2103 If you want to introduce a helper to render an icon then you should probably render the icon as an SVG image instead of the outdated, inefficient and mostly inaccessible font Icon. BTW there was a PR doing exactly that some time ago: #28351

EDIT: Just reading the code and quite frankly I don't like what I see, sorry.

• A function shouldn't have more than 3 arguments I count eight! (Uncle Bob said that not me, ref: https://www.matheus.ro/2018/01/29/clean-code-avoid-many-arguments-functions/)

A cleaner approach (more like the typo3 or my code in the #28351) would be:

function(

$icon = "",      // $icon the icon name
$as = "svg",   // $as either svg (default) or font
$params = [  //  $params array of parameters eg
  'provider'   => 'fontawesome-free',
  'group'      => 'solid',
  'classes'    => ['fa', 'demo-class'],
  'attributes' => [
   'data-foo' => 'bar',
   'id'       => 'foobar'
  ]
]) {}

• Since you're introducing an API for icons it seems that you're missing the most important aspect: decouple the assets. Right now template are dictating the behaviour (eg loading the font always) which -A. it's not template'e responsibility -B. the API should load any assets required. Eg. If my landing page doesn't have any icons why does the template should enforce the loading of unneeded css and fonts? (the template has no clue what each layout is using thus the only safe place to add assets in in the layouts NOT IN the template as these are globally loaded [monolithic approach, really outdated])

PS. Don't get me wrong here, I'm trying to suggest improvements to your code not to shame your work.

@dgrammatiko if I'm write there is no way to use svg and icon fonts at the same time (override each other with css) at least I didn't find a html markup for such a case.

Using SVG sprites doesn't seem to work or only works if the svg is inline? Loading 30 svg separated doesn't seems to be the best way.

Your PR uses inline SVG sprites right and build the svg dynamically right? doesn't sound like a smart way

Loading 30 svg separated doesn't seems to be the best way.

There aren't extra Http requests, th icons are inland in the html output, read the code/comments there

Loading 30 svg separated doesn't seems to be the best way.

There aren't extra Http requests, th icons are inland in the html output, read the code/comments there

That's what I mean with 'inline SVG sprites' you create a svg on the fly and reference them in another svg at the icon position.

That doesn't sound efficient at least you disable any caching

That doesn't sound efficient at least you disable any caching

Caching is useless if your rendering of the initial page is extremely slow but sequential loads are faster (generally speaking, for some sites this might be tolerated, eg all content is behind a login)

Anyways there are ways to do the API so you can have Icon fonts and SVG, a great example is Typo3:
https://docs.typo3.org/m/typo3/reference-coreapi/master/en-us/ApiOverview/Icon/Index.html

doesn't sound like a smart way

@HLeithner sure it's not very smart, thus everybody (GitHub, YouTube, Facebook, etc) using this approach because they are also dumb. Joomla using the outdated icon fonts is right here and the only clever... 🤷‍♂️

Can you please remove $iconPrefix = 'icon-', $iconSuffix = null, $html = true, $elem = 'span',

• We shoudn't support custom prefixes (make no sense)

We are working towards action named icons. An icon should be named save. When using this HTMLHelper it needs the icon_prefix/iconPrefix. Akeeba adds a ToolbarTitleIcon with name = akeeba. The iconPrefix is needed.

• Don't know why the suffix should be an extra parameter, also php should not be used for styling

Currently Joomla core has multiple implementations of extra parameters in the icon. Think about icon-spin, to let the icon-spinner icon animate and rotate. Think about icon-fw to give an icon a specific width.

• html parameter makes no sense if you supply the complete icon "class" (without icon-)

I agree and will remove the $html

• $elem should not be provided by php because it's a design/template decision, if I override the icon function because I want to use svg I have a problem if someone provides an unexpected markup.

I agree and will remove the $elem

• Children, don't know if this is a good idea, this would limit us by changing the iconmark to another format (which maybe can't support child elements)

I agree and will remove the $children

@dgrammatiko I prefer using svg icons too... but first things first... a HTMLHelper for the icon. Adjusting the HTMLHelper to use svg is a next step I think. The impact of this PR would be too big I think.

Looking at a11y perspective an icon should always have a text. This can be a styled label or this can be a piece of text hidden from sighted people. With the last example I mean <span class="sr-only"></span>.
With commit a3d7b64 I have added the ability to add both icon and hidden text. Example implementation can be seen on 3babe70

Looking at a11y perspective an icon should always have a text.

Like all blanket statements this is not correct

Looking at a11y perspective an icon should always have a text.

Like all blanket statements this is not correct

thank you for your comment. Can you rewrite it so it is correct?

but first things first... a HTMLHelper for the icon

Again IF you're about to introduce an new API endpoint make sure that

• The arguments could be expanded if needed (eg icon($name = 'default', $options = [])
• Properly deal with the static files, icons (either svg or font) are not responsibility of the template, should be handled here in this API
• Move the code in https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/HTML/Helpers/Icons.php or create a new file icon.php

Looking at a11y perspective an icon should always have a text.

This is a remedy. A class that hides the icon for anyone using an screen reader is a hack, svg icons don't need that and thus it's the de facto best practice for 5 or so years...

This is a remedy. A class that hides the icon for anyone using an screen reader is a hack, svg icons don't need that and thus it's the de facto best practice for 5 or so years...

Take a look at the code please. I'm not hiding the icon, I am adding a <span class="sr-only">some text here that is not visible for sighted people, but is for screenreaders</span>
Even svg icons need text. How often do I see that svg icons are optimized so much that the title element with a descriptive text has been removed from the svg. In our current Joomla code we accompany an icon with a sr-only span with the descriptive text.

The arguments could be expanded if needed (eg icon($name = 'default', $options = [])

@dgrammatiko am I not doing that with $attribs = []?

Properly deal with the static files, icons (either svg or font) are not responsibility of the template, should be handled here in this API

can you give me an example how this should be done properly?

Move the code in https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/HTML/Helpers/Icons.php or create a new file icon.php

that sounds good... I was not aware of the existence of this file.

@dgrammatiko am I not doing that with $attribs = []?

Read again my first comment, you should not spread a bunch of arguments in the function, use just 2: the icon name and then the params (an array with anything associated to with the icon you want to present). I already provided a link that explains very nicely why this is the right way...

can you give me an example how this should be done properly?

Will do

I already provided a link that explains very nicely why this is the right way...

Maybe I missed it why is this the right why? for every static code analyser it's a pain because it can not check if a parameter is valid or not and since PHP 8.0 has named parameters this shouldn't be a problem at all. But maybe I'm wrong.

And why is it not the job of the template if the icon is a svg or an icon? I mean it's easy to override this icon function with your own and replace all icon fonts with svg icons (not possible with only the template you need a plugin). But I think it's not the decision of the component if the icon is a svg icon or a font.

Maybe I missed it why is this the right why?

Argue with Uncle Bob (his book clean code is a must read for every dev): https://www.matheus.ro/2018/01/29/clean-code-avoid-many-arguments-functions/

And why is it not the job of the template if the icon is a svg or an icon?

Simply because the icons are mostly existing in layouts and layouts should take care of their dependencies (proper modularisation). Template is essentially the orchestrator, echoing the layouts in the right order, it doesn't need to have knowledge of the specifics of every or any layout. If it does it means it is enforcing things to layouts, which is not good (a very good example is #23742 )

Argue with Uncle Bob (his book clean code is a must read for every dev): https://www.matheus.ro/2018/01/29/clean-code-avoid-many-arguments-functions/

So my conclusion from this text is, that I did it right ;-) I requested to reduce the parameter count (see $html and so on). But it seems you missed the important part, if i'm not wrong he uses Java as an example in refactoring "name, phone, street..." to an Object. Your suggest was to add it to an assoc array that's not the same and error prone. The Object he created is "Address" which likely knows exactly at compile time which parameters can be added. We can do the same by creating an class "iconObject" which hold the needed information so the runtime or a static code analyzer can print an error if you have a typo. Which is not so easy to do with an assoc array.

And I think for this utility functions is a bigger pain to have an object instead of 2 additional parameters (which I would remove anyway, still don't understand why we have a suffix)

Simply because the icons are mostly existing in layouts and layouts should take care of their dependencies (proper modularisation). Template is essentially the orchestrator, echoing the layouts in the right order, it doesn't need to have knowledge of the specifics of every or any layout. If it does it means it is enforcing things to layouts, which is not good (a very good example is #23742 )

so what's the suggestion on this? I have the "idea" or some thoughts that if this PR is finished and the icon pr is finished (and if it will be merged) we should be possible to simply switch all (at least core icons) simple to svgs (which a variant of your last pr).

we should be possible to simply switch all (at least core icons) simple to svgs

That would be the perfect scenario, if we can agree on that and write some code to get us there 😉

@dgrammatiko and @HLeithner I like both of your suggestions and I look forward to the moment we can use svg instead of fonts.
Can you make a PR on my PR with your suggestions? Doing so we can all work towards a nice solution.
https://github.com/hans2103/joomla-cms/tree/htmlhelper-for-an-icon

@hans2103 small steps as you say ;-) and please move the pr to the icons htmlhelper class.

I think dimitris suggest in his edit and original PR is a bit to over-engineered, I think we don't need to add the provider i'm thinking the other way around (mainly for core), we as core provide x icon functions. If you want to use them you can use it, if you need another one you have to do your own inclusion/registration what ever. Also setting the "type" (svg/font) at the layout level is wrong and you can't override it in a template.

And if all our icons have to use such a method we hit the most complexity possible.

<?php echo HTMLHelper::_('icon.icon', 'info', 'svg', ['provider' => 'fontawesome-free', 'group' => 'solid', 'classes' => ['fa', 'demo-class'], 'attributes' => ['data-foo' => 'bar', 'id' => 'foobar']], 'sr-text' => Text::_('INFO')]); ?>

I mean nobody will use this.

@HLeithner actually if someone is using Fontawesome (due to the defaults) the code will be way simpler:

** assuming that the standard <span class"fa fas-info sr-only"><?php echo Text::_('INFO')]); ?></span> is the expected output

<?php echo HTMLHelper::_('icon.icon', 'info', 'font', [  'sr-text' => Text::_('INFO'), ]); ?>

The expected output is

<span class="fa fas-info"></span>
<span class="sr-only"><?php echo Text::_('INFO')]); ?></span>

the already existing styling for sr-only is

.sr-only {
    border: 0;
    clip: rect(0,0,0,0);
    height: 1px;
    margin: -1px;
    overflow: hidden;
    padding: 0;
    position: absolute;
    width: 1px;
    white-space: nowrap;
}

This piece of styling reduces the element to a non-visible but still existing element.
Combining sr-only with the icon will result in a non-visible icon.

Dinner now... will check on your suggestions later today or tomorrow morning. Thank you for your effort.

as a frontend developer I stumble upon my lack of decent PHP knowledge to create the content of the new /icon.php file.
@dgrammatiko and / or @HLeithner can you create a PR on this PR with your suggested /Icon.php file?

just put your function icon(...) ... code in to this one https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/HTML/Helpers/Icons.php
and then can call as

echo HTMLHelper::_('icons.icon', ....

can you create a PR on this PR with your suggested /Icon.php file

I need to do some research on the FA css/fonts, probably this weekend

Edit: it would be nice to have a pro version for checking also the files provided there

as suggested.. I've moved the function as is to libraries/src/HTML/Helpers/Icons.php
and adjusted the call with HTMLHelper::_('icons.icon', ...

ready for improvements

if the output is:

<span class="fa fas-info"></span>
<span class="sr-only"><?php echo Text::_('INFO')]); ?></span>

and the input is

<?php echo HTMLHelper::_('icons.icon', 'info', 'font', [  'sr-text' => Text::_('INFO'), ]); ?>

or even

<?php echo HTMLHelper::_('icons.icon', 'info', [ 'type' => 'font', 'sr-text' => Text::_('INFO'), ]); ?>

should we add a JLayout too where icon-prefix, extra classNames are handled?

should we add a JLayout too where icon-prefix, extra classNames are handled?

The second argument can have prefix suffix or am I getting something wrong (eg foo-icon or icon-bar)

should we add a JLayout too where icon-prefix, extra classNames are handled?

The second argument can have prefix suffix or am I getting something wrong (eg foo-icon or icon-bar)

What do you think about not handling the icon-prefix over here in the HTMLHelper, but set it using the JLayout?
That way this HTMLHelper stays a functional thing forwarding some stuff and let the JLayout handle the HTML .

Here my 2 cents, if relevant.
I also think the parameters $iconPrefix and $iconSuffix are not needed.

Let's take the change from layouts/joomla/searchtools/default/noitems.php:
The expected output for the icon is <span class="fas fa-info-circle" aria-hidden="true"></span>, but if we use your code now, we get <span class="icon-icon-fas fa-info-circle" aria-hidden="true"></span>.

This results because the prefix is defined by default and also set for a second time in $attribs['class'] = $iconPrefix . $icon;.

So if the prefix is not set by default, a developer has to consider this individually, then he can write it directly into the actual class and the parameters become unneeded. The same applies to the suffix.

Updated implementation:

<?php echo HTMLHelper::_('icons.icon', 'fas fa-info-circle', Text::_('INFO'), null, 'white'); ?>

after merge of #30707 this has to change to:

<?php echo HTMLHelper::_('icons.icon', 'icon-info-circle', Text::_('INFO'), ['class' => 'icon-white']); ?>

Method for implementation has been simplified to:
(asa @dgrammatiko suggested in #30792 (comment))

	/**
	 * Method to write a `<span>` element for an icon
	 *
	 * @param   string        $icon        The funcitonal name for an icon.
	 * @param   string        $srOnly      Screen Reader text if no visible text is placed
	 * @param   array         $attribs     Attributes to be added to the wrapping element
	 *
	 *
	 * @return  string
	 *
	 * @since   4.0
	 */
	public static function icon($icon = 'default', $srOnly = null, $attribs = [])
	{
		if (isset($attribs['class']))
		{
			$icon = $icon . ' ' . $attribs['class'];
		}

		$attribs['class'] = $icon;

		if (!isset($attribs['aria-hidden']))
		{
			$attribs['aria-hidden'] = 'true';
		}

		if (isset($srOnly))
		{
			$text = htmlspecialchars($srOnly, ENT_COMPAT, 'UTF-8');
			$srOnly = '<span class="sr-only">' . $text . '</span>';
		}

		return '<span ' . trim(ArrayHelper::toString($attribs)) . '></span>' . $srOnly;
	}

I have tested this item ✅ successfully on 846466f

@hans2103 you might find #31190 interesting.

@hans2103 you might find #31190 interesting.

Can your svg improvement be merged with this PR?

@hans2103 you might find #31190 interesting.

Can your svg improvement be merged with this PR?

baby steps.. mine seems to be up for debate as always.

Please, when modifying the lib_joomla.ini file, modify both files (admin and site).
Can't be merged until done.

@infograf768 just added the language string to the fronted with commit 631ab06

I have tested this item ✅ successfully on b1660ab

I have tested this item ✅ successfully on b1660ab

RTC

@Quy please remove Rtc from this there are a number of issues I pointed in my comments that are not addressed yet

@dgrammatiko please describe them again.
Or can they be fixed in a new PR?

It's really bad idea to merge a half done API so it cannot be done in a new pr, needs to be fixed here. Read my comment about handling the related assets and also the api needs to be able to either echo font icon or svg icon html

@dgrammatiko
do you mean this part of the comment copied from #30792 (comment)

• Since you're introducing an API for icons it seems that you're missing the most important aspect: decouple the assets. Right now template are dictating the behaviour (eg loading the font always) which -A. it's not template'e responsibility -B. the API should load any assets required. Eg. If my landing page doesn't have any icons why does the template should enforce the loading of unneeded css and fonts? (the template has no clue what each layout is using thus the only safe place to add assets in in the layouts NOT IN the template as these are globally loaded [monolithic approach, really outdated])

Adjusted icons.icon to get triggered by type = svg
Added the FontAwesome SVG icons

HELP WANTED
How to call for the FontAwesome SVG icons?
Should a reference to /media/vendor/fortawesome-free/svgs/regular be made within the function icon? => I hope not... it is not up to this function to know where the svg icons are.
Or should this be done via a JLayout where an easy override can be chosen for another library? => makes sense to me.

This PR makes even less sense now.

@SharkyKZ

This PR makes even less sense now.

could you explain me why?

closing this for now... will create a new PR when time feels good, wind is blowing from the right direction, sun is shining and such. :-)