Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0][DRAFT] replace hard coded icon by jlayout #30634

Conversation

hans2103
Copy link
Contributor

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.

@hans2103 hans2103 changed the title Feature/replace hard coded icon by jlayout [4.0][DRAFT] replace hard coded icon by jlayout Sep 14, 2020
@brianteeman
Copy link
Contributor

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

@hans2103
Copy link
Contributor Author

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.

@HLeithner
Copy link
Member

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.

@wilsonge
Copy link
Contributor

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.

@N6REJ
Copy link
Contributor

N6REJ commented Sep 15, 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.

@hans2103
Copy link
Contributor Author

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

@hans2103 hans2103 closed this Sep 15, 2020
@hans2103 hans2103 deleted the feature/replace-hard-coded-icon-by-jlayout branch November 24, 2020 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants