[4.0] Convert icon- in toolbar to fa- ( part 1 )

[N6REJ]

Pull Request for Issue #27333

Summary of Changes

change icon- classes to be consistent with using fontawesome 5 classes. ( part 1 )

Testing Instructions

apply pr
run npm ci
edit an article. you will see all the icons in the toolbar.
verify that icons have changed properly and none are missing.

Expected result

Icons are same as before the pr.

Documentation Changes Required

none

[brianteeman]

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

[Quy]

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

[N6REJ]

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.

[N6REJ]

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

[C-Lodder]

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.

[N6REJ]

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

[brianteeman]

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

[N6REJ]

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

[brianteeman]

They're not brokwn

[N6REJ]

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

[brianteeman]

Where have we done that?

[N6REJ]

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

[N6REJ]

thats just one example

[brianteeman]

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.

[N6REJ]

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

[Quy]

Remove icon?

[N6REJ]

not sure. I've not researched how that class is being used there yet. It's not used everywhere icon- is used to I felt it safer to leave it, at least for now.

[brianteeman]

It is not a vendor file.

…

On Wed, 29 Jan 2020, 21:00 Quy, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In administrator/components/com_postinstall/tmpl/messages/default.php <#27686 (comment)>: > @@ -50,7 +50,7 @@ <h2><?php echo Text::_('COM_POSTINSTALL_LBL_NOMESSAGES_TITLE'); ?></h2> <p><?php echo Text::_('COM_POSTINSTALL_LBL_NOMESSAGES_DESC'); ?></p> <a href="<?php echo Route::_('index.php?option=com_postinstall&view=messages&task=message.reset&eid=' . $this->eid . '&' . $this->token . '=1'); ?>" class="btn btn-warning btn-lg"> - <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 <#27686?email_source=notifications&email_token=AAJ4P4M6R22PKNZP7VES34DRAHU67A5CNFSM4KMLFCB2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCTRP4KQ#pullrequestreview-350420522>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ4P4LU7VRVVFFICTCOHWLRAHU67ANCNFSM4KMLFCBQ> .

[jwaisner]

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

[Quy]

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

[jwaisner]

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.

---

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

[Quy]

I have tested this item ✅ successfully on b913de3

---

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

[jwaisner]

RTC

---

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

[wilsonge]

Thanks guys. Nice work @N6REJ

[N6REJ]

ty george