are you planning to convert all the instances of icon- ??

seems a waste of effort to do this as the icon that is rendered is the fontawesome icon

Yes, please read the following comment: #27333 (comment)

are you planning to convert all the instances of icon- ??

seems a waste of effort to do this as the icon that is rendered is the fontawesome icon

because of things like this I'm not sure it's entirely realistic to try to change all in a single pr.. "icon-new" is a great example of the problem.

@Quy I think we need to wait for #27657 to get merged before moving forward so we are not chasing our tail

seems a waste of effort to do this as the icon that is rendered is the fontawesome icon

@brianteeman The icon-* mappings were there for extension developers to maintain B/C. Ideally they should be made deprecated in 4.0 and removed in 5.0. So moving core to the FA classes would be best done now rather than later.

Of course, the deprecation for icon-* is just my opinion. If it were up to me, those mappings wouldn't even be there.

seems a waste of effort to do this as the icon that is rendered is the fontawesome icon

@brianteeman The icon-* mappings were there for extension developers to maintain B/C. Ideally they should be made deprecated in 4.0 and removed in 5.0. So moving core to the FA classes would be best done now rather than later.

Of course, the deprecation for icon-* is just my opinion. If it were up to me, those mappings wouldn't even be there.

I checked with @wilsonge and can confirm they will be gone in J! 5

so just mark them as deprecated and get on with something more useful. it really is just renaming for the sake of renaming

so just mark them as deprecated and get on with something more useful. it really is just renaming for the sake of renaming

so, your suggestion is to mark them deprecated but NOT fix them?
Is that truly constructive?.

They're not brokwn

right, but only because we've bastardized fontawesome & when we do remove them in 5 all this work will need to be done.

Where have we done that?

you'll see here we've added icon-options to fontawesome

thats just one example

That is what is generated by the scss - nothing wrong there at all - it is exactly what you are supposed to do.

As @C-Lodder said before it is there for backwards compatibility so that developers do not have to change all their icons if they dont want to.

no, that is vendor file. It should be sacrosanct and what icomoon map is for. Then that is compiled into template.css

It is not a vendor file.

On Wed, 29 Jan 2020, 21:00 Quy, notifications@github.com wrote:

@Quy commented on this pull request.

In administrator/components/com_postinstall/tmpl/messages/default.php
#27686 (comment):

@@ -50,7 +50,7 @@

• 	<span class="icon icon-eye-open" aria-hidden="true"></span>
  

• 	<span class="icon fas fa-eye" aria-hidden="true"></span>
  

Remove icon?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/joomla/joomla-cms/pull/27686?email_source=notifications&email_token=AAJ4P4M6R22PKNZP7VES34DRAHU67A5CNFSM4KMLFCB2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTRP4KQ#pullrequestreview-350420522,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAJ4P4LU7VRVVFFICTCOHWLRAHU67ANCNFSM4KMLFCBQ
.

@N6REG that's not the vendor. The mappings extend the same styling using SCSS's @extend method. You'll notice a comma between the classes by which they both use the styling from that file ;)

Either way, using FA classes in core now isn't hurting anyone. No reason not to do it now, then in a couple of decades when J5 is ready, the SCSS file simply needs removing.

Thanks @C-Lodder. As @N6REJ doesn't believe anything I say hopefully he will listen to you. I just don't have the patience to explain how scss works to a front-end developer

@C-Lodder this is why I'm saying its a vendor file..

I see the icomoon.scss which is doing mapping...

so, are you saying, we're taking icomoon.scss, fontawesome 5 scss & putting the compiled version in vendor/fontawesome.css ?
why in the vendor folder? Thats confusing.
btw, I saw the comma so I understand how that part is working.

Thanks @C-Lodder. As @N6REJ doesn't believe anything I say hopefully he will listen to you. I just don't have the patience to explain how scss works to a front-end developer

@brianteeman because MOST of the time instead of being constructive & helpful, you are negative and condescending towards ANYTHING I try to do.

@N6REJ have a look at this file (which belongs to the template): https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/templates/atum/scss/vendor/fontawesome-free/fontawesome.scss

You'll notice it imports the FA vendor and icomoon mapping. It is then compiled as a separate file to the template CSS: https://github.com/joomla/joomla-cms/blob/4.0-dev/build/build-modules-js/compilecss.es6.js#L55

FA do not use any icon- prefixes

FA do not use any icon- prefixes
100% correct.

This is part 1 of the "fix". Reason for stopping here is because the next step is going to be much more complicated to fix as its not simple changes. So I figure better to have separate pr for that one.

how do I reset this to draft?

READY FOR TESTING
I'm leaving icon-new and icon-option and maybe a couple more for another pr since its not simple <span class=
its icon-$name and it's not clear to me where & how that is done YET.

@N6REJ Based on the referenced issue the PR doesnt seem to change the issue with the icons in the toolbar. It doesnt break anything but the gap at the bottom still exits.

Please do correct me if I misunderstand the intended result.

@jwaisner That is a separate issue to address later. This PR is to convert icon to fa. See #27333 (comment)

I have tested this item ✅ successfully on b913de3

Tested all icons throughout including a clean install. No issues found with the display related to this PR.

I have tested this item ✅ successfully on b913de3

RTC

Thanks guys. Nice work @N6REJ

ty george