? Pending

User tests: Successful: Unsuccessful:

avatar hans2103
hans2103
14 Sep 2020

Pull Request for Issue # .

Summary of Changes

With this PR merged all hard coded icons <span class="fas fa-some-kind-of-icon" aria-hidden="true" and-more-attributes></span> are being replaced by the JLayout implemented with #30470
The reason behind this is to make Joomla more flexible to switch to other icon libraries, or to implement your own.
With this PR merged the declaration of icons is only be done in https://github.com/joomla/joomla-cms/blob/4.0-dev/layouts/joomla/icon/iconclass.php

Testing Instructions

  • Open Joomla Administrator and inspect icons
  • Apply PR and refresh page
  • All icons should be shown as it was before this PR.

Actual result BEFORE applying this Pull Request

All icons are hard coded.

Expected result AFTER applying this Pull Request

Most of all icons are set through the LayoutHelper('joomla.icon.iconclass', ['icon' => 'some-kind-of-icon-type'])

Documentation Changes Required

I don't know if there is documentation about the present LayoutHelpers and how they are / should be used.

avatar hans2103 hans2103 - open - 14 Sep 2020
avatar hans2103 hans2103 - change - 14 Sep 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 14 Sep 2020
Category Administration com_admin com_associations com_banners com_cache com_categories com_config com_contact com_content com_contenthistory com_cpanel com_csp com_fields com_finder
avatar hans2103 hans2103 - change - 14 Sep 2020
Labels Added: ?
avatar hans2103 hans2103 - change - 14 Sep 2020
Title
Feature/replace hard coded icon by jlayout
[4.0][DRAFT] replace hard coded icon by jlayout
avatar hans2103 hans2103 - edited - 14 Sep 2020
avatar brianteeman
brianteeman - comment - 14 Sep 2020

not at my pc yet so quick questions
what about the old style of icons used by extensions - I assume they still work?
there are some places where the icon is set/changed in js eg show password

avatar hans2103
hans2103 - comment - 14 Sep 2020

what about the old style of icons used by extensions - I assume they still work?

I'm not changing the look and feel of rendering the HTML output for example<span class="fas fa-spinner" aria-hidden="true"></span>. Only the way Joomla core adds an icon in the core by using a JLayout.

there are some places where the icon is set/changed in js eg show password

I know... haven't found out how to tackle that part, so I skipped it for a later change.

avatar joomla-cms-bot joomla-cms-bot - change - 14 Sep 2020
Category Administration com_admin com_associations com_banners com_cache com_categories com_config com_contact com_content com_contenthistory com_cpanel com_csp com_fields com_finder Administration com_admin com_associations com_banners com_cache com_categories com_config com_contact com_content com_contenthistory com_cpanel com_csp com_fields
avatar HLeithner
HLeithner - comment - 14 Sep 2020

That PR looks like an performance nightmare and makes templates much more complex.

If someone want's to replace the Icons I would suggest to override the template or the css classes.

avatar wilsonge
wilsonge - comment - 14 Sep 2020

I'm not convinced by this in the admin - I'm not so fussed about coupling to font awesome there. Can see the potential win in frontend however.

avatar N6REJ
N6REJ - comment - 15 Sep 2020

@HLeithner is there a test that can be run to measure performance other then on a page by page basis?
I huge part of the problem is the restriction that we must maintain b/c.

avatar hans2103
hans2103 - comment - 15 Sep 2020

That PR looks like an performance nightmare and makes templates much more complex.

If someone want's to replace the Icons I would suggest to override the template or the css classes.

Wasn't this the whole purpose of JLayouts? To code DRY. Replacing hard coded icons by a JLayout will make it DRY. One place where you control the icons used, instead of scattered around the entire project.

I'm not convinced by this in the admin - I'm not so fussed about coupling to font awesome there. Can see the potential win in frontend however.

Since you're the one with the force to merge it branch 4.0-dev there is no use to continue with this PR. I'll close it, but will create a new PR where the JLayout is extended for the use of frontend

avatar hans2103 hans2103 - change - 15 Sep 2020
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2020-09-15 06:12:29
Closed_By hans2103
avatar hans2103 hans2103 - close - 15 Sep 2020

Add a Comment

Login with GitHub to post a comment