[4.0] Created a HTMLHelper to render an icon #30792

hans2103 · 2020-09-28T07:55:55Z

Pull Request for Issue # .

Summary of Changes

This PR will make it possible to use a HTMLHelper to place an icon.
Just like there is a HTMLHelper to place an anchor and an image.

anchor =>

joomla-cms/libraries/src/HTML/HTMLHelper.php

Line 253 in b2e4bf0

public static function link($url, $text, $attribs = null)
image =>

joomla-cms/libraries/src/HTML/HTMLHelper.php

Line 574 in b2e4bf0

public static function image($file, $alt, $attribs = null, $relative = false, $returnPath = 0)

Testing Instructions

Joomla 4 Administrator
See the quick icons on the dashboard
Apply patch
See the quick icons on the dashboard ... it is still the same. Only to be implemented by the HTMLHelper now

Actual result BEFORE applying this Pull Request

placing an icon would be:

<span class="icon-joomla" aria-hidden="true"></span>
<span class="icon-joomla icon-white" aria-hidden="true"></span>
<span class="icon-joomla icon-white" aria-hidden="true" tabindex="0" title="Hello World"></span>
icon-joomla
<span class="fab fa-joomla" aria-hidden="true"></span>

Expected result AFTER applying this Pull Request

echo HTMLHelper::_('icons.icon', 'joomla', TEXT::_('HELLO_WORLD'));

Documentation Changes Required

most likely...

SharkyKZ · 2020-09-28T08:46:39Z

This looks pointless and counter-intuitive.

layouts/joomla/toolbar/title.php

libraries/src/HTML/HTMLHelper.php

…366479

libraries/src/HTML/HTMLHelper.php

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

HLeithner · 2020-09-28T11:43:45Z

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)

dgrammatiko · 2020-09-28T13:28:37Z

@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.

HLeithner · 2020-09-28T13:36:27Z

@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

dgrammatiko · 2020-09-28T13:40:09Z

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

HLeithner · 2020-09-28T13:47:09Z

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

dgrammatiko · 2020-09-28T14:05:48Z

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... 🤷‍♂️

hans2103 · 2020-09-30T22:17:50Z

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

hans2103 · 2020-10-01T06:29:00Z

@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

brianteeman · 2020-10-01T07:32:09Z

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

Like all blanket statements this is not correct

hans2103 · 2020-10-01T07:52:37Z

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?

dgrammatiko · 2020-10-01T10:01:35Z

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...

hans2103 · 2020-10-01T11:19:19Z

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 · 2020-10-01T11:25:05Z

@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

HLeithner · 2020-10-01T11:29:45Z

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.

dgrammatiko · 2020-10-01T11:36:54Z

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 )

HLeithner · 2020-10-01T12:05:21Z

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).

# Conflicts: # layouts/joomla/searchtools/default/noitems.php # layouts/joomla/toolbar/title.php

Quy · 2020-10-28T22:52:02Z

I have tested this item ✅ successfully on b1660ab

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30792.}

N6REJ · 2020-10-30T22:47:36Z

I have tested this item ✅ successfully on b1660ab

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30792.}

Quy · 2020-10-31T00:49:16Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30792.}

dgrammatiko · 2020-10-31T08:16:35Z

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

hans2103 · 2020-10-31T09:43:13Z

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

dgrammatiko · 2020-10-31T09:46:49Z

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

hans2103 · 2020-10-31T10:13:37Z

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

# Conflicts: # libraries/src/HTML/Helpers/Icons.php

…rd should be visible if helper is finished

hans2103 · 2020-11-26T10:31:21Z

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.

build/build-modules-js/settings.json

dgrammatiko · 2020-11-26T10:33:37Z

libraries/src/HTML/Helpers/Icons.php

+	 *
+	 * @since   __DEPLOY_VERSION__
+	 */
+	public static function icon(string $icon, string $srOnly = '', string $type = '', array $attribs = []): string


Please remove the srOnly or move it as a param to attribute. Actually please bring the logic from my PR https://github.com/joomla/joomla-cms/pull/28351/files#diff-1f3d4b0ff411d275d94c1538ef487ef75d4450d772ad2cf712b5796467917f6dR214-R246
That allows any type of attributes to be attached to the inserted svg...

applied your changes... some of them.
I don't want to use svg-sprite at this moment... out of scope for this change.

But.. I need some help to get provide the correct path to the function icons.svg

suggestions?

I don't want to use svg-sprite at this moment... out of scope for this change.

You're missing the point my friend, if you're not controlling the output (eg current implementation) you're essentially allowing broken HTML to be applied in any page. Let me explain: the svg helper will echo each fragment every time you call it, meaning it can inflate the output but worse than that it could echo multiple svgs with the same id which is invalid HTML. Unfortunately if you want to do SVGs correctly you need somehow to take control of the fragments you're inserting or the API you're introducing is prone to undesired side effects

SharkyKZ · 2020-11-26T11:52:39Z

This PR makes even less sense now.

libraries/src/HTML/Helpers/Icons.php

hans2103 · 2020-11-26T12:19:59Z

@SharkyKZ

This PR makes even less sense now.

could you explain me why?

hans2103 · 2021-05-11T06:47:44Z

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. :-)

hans2103 added 3 commits September 28, 2020 09:33

Created a HTMLHelper to render an icon

c14be3c

clean code

7b27c78

remove hard coded icon

7d651df

joomla-cms-bot added the PR-4.0-dev label Sep 28, 2020

hans2103 changed the title ~~Created a HTMLHelper to render an icon~~ [4.0] Created a HTMLHelper to render an icon Sep 28, 2020

Fedik reviewed Sep 28, 2020

View reviewed changes

layouts/joomla/toolbar/title.php Outdated Show resolved Hide resolved

richard67 reviewed Sep 28, 2020

View reviewed changes

libraries/src/HTML/HTMLHelper.php Outdated Show resolved Hide resolved

https://github.com/joomla/joomla-cms/pull/30792#pullrequestreview-497…

67495f6

…366479

richard67 reviewed Sep 28, 2020

View reviewed changes

libraries/src/HTML/HTMLHelper.php Outdated Show resolved Hide resolved

hans2103 and others added 2 commits September 28, 2020 13:31

Update libraries/src/HTML/HTMLHelper.php

37679ba

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

Update libraries/src/HTML/HTMLHelper.php

de4d0a1

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>

hans2103 added 2 commits October 1, 2020 08:24

remove $html and $children. Add $srOnly

a3d7b64

apply HTMLHelper icon including sr-only

3babe70

hans2103 added 3 commits October 27, 2020 11:36

Merge branch 'upstream/4.0-dev' into feature/htmlhelper-for-an-icon

f884d3f

Merge branch 'upstream/4.0-dev' into feature/htmlhelper-for-an-icon

c6e07c6

# Conflicts: # layouts/joomla/searchtools/default/noitems.php # layouts/joomla/toolbar/title.php

reapply change

8947ace

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 31, 2020

Quy removed the RTC This Pull Request is Ready To Commit label Oct 31, 2020

hans2103 added 5 commits November 26, 2020 10:47

Merge branch 'upstream/4.0-dev' into htmlhelper-for-an-icon

d37a47c

# Conflicts: # libraries/src/HTML/Helpers/Icons.php

FontAwesome provides svg icons... use them

2fd362e

no idea how to explain the test instructions for this. :-)

8ddfcde

easier for test instructions... after apply path the icons on dashboa…

7043049

…rd should be visible if helper is finished

add new parameter $type to trigger svg icons

413191f

hans2103 marked this pull request as draft November 26, 2020 10:33

dgrammatiko suggested changes Nov 26, 2020

View reviewed changes

hans2103 added 3 commits November 26, 2020 11:34

adjust settings.json

83551d9

call helper icons.icon with svg

3b0314c

optimize icons.icon for svg

c91b14c

brianteeman reviewed Nov 26, 2020

View reviewed changes

libraries/src/HTML/Helpers/Icons.php Outdated Show resolved Hide resolved

only set sr-only text when aria-hidden is true

4a765c5

hans2103 closed this May 11, 2021

hans2103 deleted the htmlhelper-for-an-icon branch September 10, 2021 06:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4.0] Created a HTMLHelper to render an icon #30792

[4.0] Created a HTMLHelper to render an icon #30792

hans2103 commented Sep 28, 2020 •

edited

SharkyKZ commented Sep 28, 2020

HLeithner commented Sep 28, 2020

dgrammatiko commented Sep 28, 2020 •

edited

HLeithner commented Sep 28, 2020

dgrammatiko commented Sep 28, 2020

HLeithner commented Sep 28, 2020

dgrammatiko commented Sep 28, 2020 •

edited

hans2103 commented Sep 30, 2020

hans2103 commented Oct 1, 2020

brianteeman commented Oct 1, 2020

hans2103 commented Oct 1, 2020

dgrammatiko commented Oct 1, 2020 •

edited

hans2103 commented Oct 1, 2020

dgrammatiko commented Oct 1, 2020

HLeithner commented Oct 1, 2020

dgrammatiko commented Oct 1, 2020

HLeithner commented Oct 1, 2020

Quy commented Oct 28, 2020

N6REJ commented Oct 30, 2020

Quy commented Oct 31, 2020

dgrammatiko commented Oct 31, 2020

hans2103 commented Oct 31, 2020

dgrammatiko commented Oct 31, 2020

hans2103 commented Oct 31, 2020

hans2103 commented Nov 26, 2020 •

edited

dgrammatiko Nov 26, 2020

hans2103 Nov 26, 2020

dgrammatiko Nov 26, 2020

SharkyKZ commented Nov 26, 2020

hans2103 commented Nov 26, 2020

hans2103 commented May 11, 2021

[4.0] Created a HTMLHelper to render an icon #30792

[4.0] Created a HTMLHelper to render an icon #30792

Conversation

hans2103 commented Sep 28, 2020 • edited

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

SharkyKZ commented Sep 28, 2020

HLeithner commented Sep 28, 2020

dgrammatiko commented Sep 28, 2020 • edited

HLeithner commented Sep 28, 2020

dgrammatiko commented Sep 28, 2020

HLeithner commented Sep 28, 2020

dgrammatiko commented Sep 28, 2020 • edited

hans2103 commented Sep 30, 2020

hans2103 commented Oct 1, 2020

brianteeman commented Oct 1, 2020

hans2103 commented Oct 1, 2020

dgrammatiko commented Oct 1, 2020 • edited

hans2103 commented Oct 1, 2020

dgrammatiko commented Oct 1, 2020

HLeithner commented Oct 1, 2020

dgrammatiko commented Oct 1, 2020

HLeithner commented Oct 1, 2020

Quy commented Oct 28, 2020

N6REJ commented Oct 30, 2020

Quy commented Oct 31, 2020

dgrammatiko commented Oct 31, 2020

hans2103 commented Oct 31, 2020

dgrammatiko commented Oct 31, 2020

hans2103 commented Oct 31, 2020

hans2103 commented Nov 26, 2020 • edited

dgrammatiko Nov 26, 2020

Choose a reason for hiding this comment

hans2103 Nov 26, 2020

Choose a reason for hiding this comment

dgrammatiko Nov 26, 2020

Choose a reason for hiding this comment

SharkyKZ commented Nov 26, 2020

hans2103 commented Nov 26, 2020

hans2103 commented May 11, 2021

hans2103 commented Sep 28, 2020 •

edited

dgrammatiko commented Sep 28, 2020 •

edited

dgrammatiko commented Sep 28, 2020 •

edited

dgrammatiko commented Oct 1, 2020 •

edited

hans2103 commented Nov 26, 2020 •

edited