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

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

@Quy How that? This is new to me.

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

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

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.

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.

Maybe @C-Lodder could review?

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

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

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

Yes for core only and keep mapping.

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

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

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 I've found places in scss where we need to correct selectors. I'll make a PR for your branch of this PR later so you can merge with 1 click. But now I have to go for a few hours and do something else.

When changing css selectors in scss I will add the new ".fas" selector in addition to the ".fa", not replace the ".fa", so we stay compatible for 3rd party extensions. In general that's how it is already done e.g. in ./media/vendor/fontawesome-free/scss/solid.scss:

.fa,
.fas {
...

Another thing is that someone should check javascript if we use the "fa" selector there somewhere to retrieve elements.

@N6REJ Have tried to fix scss but I wasn't successful yet. Have to stop right now. Maybe will try later again.

.,

When changing css selectors in scss I will add the new ".fas" selector in addition to the ".fa", not replace the ".fa", so we stay compatible for 3rd party extensions. In general that's how it is already done e.g. in ./media/vendor/fontawesome-free/scss/solid.scss:

.fa,
.fas {
...

added quickicons fix in latest commit

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

yeah, I see that " @extend .#{$fa-css-prefix}" will need modifying. Can we fix that after this fix? Or does it need to be done simultaneously?
so, administrator\templates\atum\scss_variables.scss line 125 needs to change to "$fa-css-prefix: fas;"

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

ty brian, fixed.

Can we fix that after this fix? Or does it need to be done simultaneously?

It is an essential part of this

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

done 65c6961

I believe

It is solid and not regular.

https://fontawesome.com/icons/eye-slash?style=solid

Missing lock icon next to Complete & Open Admin.

It is solid and not regular.

https://fontawesome.com/icons/eye-slash?style=solid

would you please do a visual compare between fa-4 and fa-5 for that. According to the content and comment in fa-5 code its far.

Missing lock icon next to Complete & Open Admin.

you know which file that is?

would you please do a visual compare between fa-4 and fa-5 for that. According to the content and comment in fa-5 code its far.

There are solid and regular versions of eye and eye-slash. Refer to Font Awesome.

https://fontawesome.com/icons/eye
https://fontawesome.com/icons/eye-slash

Fairly certain that the package lock should not be included in this PR

would you please do a visual compare between fa-4 and fa-5 for that.

Please stop referring to fa-4 as it is not correct. All the changes required (including renamed fonts) from 4 to 5 were already addressed. This PR is only about changing to not use a deprecated class.

would you please do a visual compare between fa-4 and fa-5 for that.

Please stop referring to fa-4 as it is not correct. All the changes required (including renamed fonts) from 4 to 5 were already addressed. This PR is only about changing to not use a deprecated class.

as @C-Lodder and I have both said thats not an entirely true statement. "fa fa-eye" ( for example ) is a fa-4 icon. in fa-5 it's actually "far fa-eye". This is NOT how it currently is without this pr ergo the conversion to fa-5 was not fully implemented.
I really don't see what the issue with what verbage is used as it basically amounts to the same thing. The deprecated class names ( i.e fa-4 classes ) need to be corrected to current classes ( i.e. fa-5)

would you please do a visual compare between fa-4 and fa-5 for that. According to the content and comment in fa-5 code its far.

There are solid and regular versions of eye and eye-slash. Refer to Font Awesome.

https://fontawesome.com/icons/eye
https://fontawesome.com/icons/eye-slash

exactly. If we do a content code compare, MOST are fas but a few ( like the eyes ) are far.

exactly. If we do a content code compare, MOST are fas but a few ( like the eyes ) are far.

Actually that is not the case with eyes. It has both.

Here's a visual example.
This was "fa fa-eye" in fa-4. Notice now it even states that THIS is the actual replacement not "fas fa-eye"

exactly. If we do a content code compare, MOST are fas but a few ( like the eyes ) are far.

Actually that is not the case with eyes. It has both.

apparently your correct... same content code different visual.
so, do we want to make a decision and make all the icons .fas irregardless of visual change?
if my eyes aren't wrong ( and they could be ) isn't far the same visual as fa-4?

OK I understand now your point. To keep it simple, I would say use fas only.

Fairly certain that the package lock should not be included in this PR

@brian where, and how would I remove it at this point?

@wilsonge what do we do about the supposed conflict? I'm not understanding how its a conflict when its a change that needs to be done.

@N6REJ Conflict because in the 4.0-dev branch changes to the same file have been merged, changes maybe even at the same place in the file. Have no time now to explain solving conflicts, am at work.

@N6REJ @brianteeman means the "administrator/components/com_media/package-lock.json" file. It is included in your PR. to get it off the PR, select it in your git client and select to revert the changes in that file (if there is such option). If you can't do that, try to check out the file again. And if that doesn't help, dowload the file from the 4.0-dev branch and put it into your local environment at the right place, then your git client should see it is the same as in current base branch (=4.0-dev). Good luck with all.

the conflict is because the version of the file that you have edited in your local branch is not the same version as the file here in the 4.0-dev branch

Change in Media:

Change in Media:

FIXED IN: 0ac0c8b
what do you do with the package-lock.json since running npm i is required to see the change?

I have tested this item ✅ successfully on cf92b74

I have tested this item ✅ successfully on cf92b74

I started to write this inline but its generic so writing here instead.

The change for the font in the markup seems ok but I am not so sure about the scss

1. as you're removing all the instance of .fa is it still needed to have .fa defined in the scss

2. you've replaced fa with fas but shouldnt that also include far and fab

as you're removing all the instance of .fa is this still needed ?

@brianteeman That could be my fault. I had suggested that at the beginning for being bc with 3rd party extensions (modules, ...) still using "fa". But now I think that at least at these places it does not really make sense.

@N6REJ Sorry for wrong suggestion.

@richard67 I am just asking the question - whatever the answer it must be consistent

far
You are using .far a few times in the scss but I dont see media/vendor/fontawesome-free/scss/regular being included in the administrator\templates\atum\scss\vendor\fontawesome.scss

far
You are using .far a few times in the scss but I dont see media/vendor/fontawesome-free/scss/regular being included in the administrator\templates\atum\scss\vendor\fontawesome.scss

you bring up a good point. Looking more closely, I don't see .far even being used. so it makes no sense to have it included. Same with .fal & .fad

@richard67 I am just asking the question - whatever the answer it must be consistent

.fa is included as part of the solid.scss so perhaps that one should stay.

@Quy @C-Lodder @brianteeman @wilsonge Let's come to a consensus.
currently the brand & solid sets are the only 2 sets being used.
that means .fa, .fas, & .fab are the only valid classes.
we are NOT using .far so we either need to add it to https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/templates/atum/scss/vendor/fontawesome-free/fontawesome.scss OR not support .far in the backend which would(?) effect 3rd party extensions.
.fal & .fad are also valid classes but they are paid classes and frankly we don't need them.
.fa is deprecated and mapped to .fas. So I don't think we'd lose anything by removing it.
.fas MUST stay.
.fab is used for joomla icon.
.far unused
.fal & .fad not included in fontawesome free.
so, do we want...

1. .fa, .fas, .far, .fab
2. .fa, .fas, .fab
3. .fas, .fab

.fa is deprecated and mapped to .fas. So I don't think we'd lose anything by removing

It's the other way around. .fas is mapped to ..fa
Isn't it?

.fa is deprecated and mapped to .fas. So I don't think we'd lose anything by removing

It's the other way around. .fas is mapped to ..fa
Isn't it?

well, in solid.scss you have

According to fontawesome docs .fas is the valid set not .fa

I guess we have a different understanding of what mapped means - perhaps lost in translation

.fa is deprecated and mapped to .fas. So I don't think we'd lose anything by removing it.

I read that as you saying - we can remove .fa from our css because if someone did use it in their code it would still work because it is mapped. And I am saying that this isn't true.

Also just a reminder that production said it was ok to be hard coded to fontawesome in the admin but not in the frontend where the markup should allow a designer to use their own icon font easily.

I read that as you saying - we can remove .fa from our css because if someone did use it in their code it would still work because it is mapped. And I am saying that this isn't true.

I think I get what your saying... Please correct me if I'm wrong...
while its true .fa & .fas do exist together in solid.scss if we don't include .fa in our ... overrides?... then it will only be the default behavior.
OR, perhaps i'm missing something altogether?

Nothing to do with overrides

What I am saying is that this statement is incorrect

.fa is deprecated and mapped to .fas. So I don't think we'd lose anything by removing it.
if you remove .fa

then any use of fa will not have that styling applied.

Honestly it really is taking a lot of time to "fix" something that doesn't need fixing

Nothing to do with overrides

What I am saying is that this statement is incorrect

.fa is deprecated and mapped to .fas. So I don't think we'd lose anything by removing it.
if you remove .fa

then any use of fa will not have that styling applied.

Honestly it really is taking a lot of time to "fix" something that doesn't need fixing

maybe your right about time. But thats one thing I have and at least I'm fairly knowledgeable about fontawesome. It WILL need to be done by J!5.
as for the other, so are you saying we need to keep .fa?
what about when 5 comes?

what about when 5 comes?

J4 has been 3 years in the making

@N6REJ I've just started to test with a side-by-side comparison of two testing environment, one with and one without this PR. I've noticed that the green colour of the update checks quickicons is too dark with your PR, except of the one for Joomla which is ok:

When hovering the icons, colors are ok, i.e. same as before patch and same as for Joomla icon after patch.
Can you check that and confirm or not?

@N6REJ Found reason for my above comment: It seems you missed a few changes of "fa" to "fas" in administrator/templates/atum/scss/blocks/_quickicons.scss. Will check and make PR to your branch.

@N6REJ See N6REJ#10 .

I have tested this item ✅ successfully on 97d8d18

I have not tested this item.

I have tested this item ✅ successfully on 97d8d18

Had forgotten to test installation form, that's why I had reset my testing result before.
Now all tested in a side-by-side comparion of 2 test installations, one without and one with this PR.
Installation form, backend and frontend tested.
No visible differences in icons. I hope I haven't missed anything.

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

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

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

I have tested this item ✅ successfully on 97d8d18

@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

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

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

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

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.

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

I have tested this item ✅ successfully on 97d8d18

RTC

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

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)

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

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

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