[4.0] - Update fontawesome 4- to fontawesome 5 classes

[N6REJ]

Pull Request for Issue #27655

Summary of Changes

replaces fontawesome 4 classes with fontawesome 5 equivalents.

Testing Instructions

Apply PR
run npm i
delete configuration.php file
reinstall
Browse frontend and backend to make sure no visible changes with the icons.

Expected result

icons display as previous.

Actual result

Documentation Changes Required

none.

[Quy]

Remove Run npm i since no longer required when doing a reinstall.

[richard67]

Remove Run npm i since no longer required when doing a reinstall.

@Quy How that? This is new to me.

[Quy]

You’re right. Too early here...brain not up to speed.

[brianteeman]

If the intention of this pr is to remove all instance of .fa and for all the icons still to work then this PR is not correct. Take a look at the original PR that upgraded from 4 to 5 and you will see why. As I said before a search and replace alone will not achieve what you are aiming to do

[richard67]

Sure I will check that. Give me some time. I hope to have reviewed all end of today. The original PR for the update I remember well, and I once had to do the same for another software not related to Joomla. So I will go through it icon by icon.

What can be that we (Joomla) use the old style ".fa" somwhere as selector in (s)css where we should have changed it to new selectors, which would not have been trivial because it could require changes on (s)css structures. So yes, this PR has to be tested with care.

[N6REJ]

So yes, this PR has to be tested with care.

YES PLEASE! there are a LOT of changes done in lots of places so would be easy to make a mistake SOMEWHERE.

[richard67]

Maybe @C-Lodder could review?

[brianteeman]

On the left is the result of using fa
On the right is the result of using fas

Below is the effect of this PR on the update checks module

In addition you need to review and update the variables file for the template and check the icomoon mapping

[Quy]

@N6REJ if you have the time, would you do a PR to change icon- to fas- discusses here #27333 (comment) ? Thanks.

[brianteeman]

@Quy that is fine to do for core but it still needs to be supported for extensions unless we want to break them all - thats why there is a mapping (created by @C-Lodder )

[Quy]

Yes for core only and keep mapping.

[C-Lodder]

I believe there are several instances of targeting .fa in the SCSS, so these will need to be changed too, which will probably fix the issue in Brians screenshot.

Wish people has listened to my comments about FA ages ago, but oh well.

I will code review this PR but wont do any observational tests

[richard67]

I'm busy now with family stuff for the afternoon but will later help where I can.

[richard67]

As a reference, the original PR which was doing the upgrade of Fontawesome from 4 to 5 and which was mentioned in discussion in issue #27655 is PR #24648 .

[N6REJ]

@richard67 I THINK we've covered everything now... least I hope so.

[richard67]

@N6REJ Why you merged 4.0-dev? There was no reason for it. Now I again have to set my test result. And it also needs resources on our testing environment to run the tests.

[richard67]

It seems the issue tracker kept the test result .. strange, normally it was lost after an update to base branch.

[richard67]

I have tested this item ✅ successfully on 97d8d18

---

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

[N6REJ]

@N6REJ Why you merged 4.0-dev? There was no reason for it. Now I again have to set my test result. And it also needs resources on our testing environment to run the tests.

sorry didn't know, thought I had to keep it up to date

[richard67]

@N6REJ No, as long as GitHub doesn’t show conflicts it isn’t necessary. We do that sometimes though in order to restart integration tests in case if they failed before so we can check if really not related to the PR (sometimes they fail for unrelated reasons), but that was also not the case here. Is not a big problem but it makes test count in GitHub be reset. In the issue tracker it was still ok.

[richard67]

@N6REJ Due to recently merged corrections for LTR of the installation form there are conflicts now for file installation/template/scss/template-rtl.scss. Please resolve them. If necessary I can advise on Glip. Thanks in advance.

[brianteeman]

Just a note for the future.

We cannot remove the mapping of the icomoon font classes to the fontawesome classes as this is still used extensively in core

[richard67]

I have tested this item ✅ successfully on 97d8d18

Test is still good because last merges were only for updating to base branch including solving merge conflicts.
Because this happened with some struggle I have explicitely tested with success that the changes from #27732 haven't been lost.
To me it seems all fine now.

---

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

[richard67]

@Quy or @jwaisner could you test again? And if possible test new installation, too, for the installation form? And also test RTC for all, installation form, frontend, backend? Thanks in advance.

[Quy]

I have tested this item ✅ successfully on 97d8d18

---

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

[Quy]

RTC

---

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

[brianteeman]

you got their in the end @N6REJ ;) 🥇 🥇

[richard67]

I currently think about making a pr to change it back from "fas" to "fa" to make our sources a bit smaller and so save disk space 😛 (joking)

[brianteeman]

It would save bytes and reduced the cost to the users for their data transfer

[N6REJ]

you got their in the end @N6REJ ;) 🥇 🥇

lol ty brian.
ty all, now I can work on part 2