NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar Quy
Quy
20 May 2021

Summary of Changes

Change the last instances of fa-.

Change fa-8x to icon-8x.
Apply coding style.

Testing Instructions

Install the prebuilt package.

or

Click Post Installation Messages icon.
Click Hide all messages button.
Apply PR.
Run npm run build:css

Bell icon should be the same size before/after PR.

avatar Quy Quy - open - 20 May 2021
avatar Quy Quy - change - 20 May 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 20 May 2021
Category Administration com_postinstall Repository NPM Change Layout
avatar brianteeman
brianteeman - comment - 20 May 2021
  1. repeating my objection to this pointless change
  2. its only a partial job - what if in 2 days someone submits a pr with fa-7x or any other valid and working fa class that has not been duplicated as per this pr
  3. <span class="fa-8x mb-4 <?php echo $icon; ?>" aria-hidden="true"></span>
avatar Quy
Quy - comment - 20 May 2021

Then they can use fa-7x, but at least in the core, we are being consistent.

avatar C-Lodder
C-Lodder - comment - 20 May 2021

@Quy You want to start moving away from icon-xx. It was kept purely for 3rd party compatability reasons. There is absolutely no reason for core to use icon- over fas-

avatar richard67
richard67 - comment - 20 May 2021
avatar Quy
Quy - comment - 20 May 2021

PR #30707 changed fas- to icon-. Is it too late to revert before RC?

avatar brianteeman
brianteeman - comment - 20 May 2021

That other pr was reverting poorly implemented code despite multiple people pointing that out in advance ;(

icon- was created to be a bc compatibility layer with iconmoon. it was never intended to be a conversion between fontawesome syntax and iconmoon syntax which is what this pr (and others) are doing

avatar Quy
Quy - comment - 20 May 2021

Let's do it right out of the gate.

@wilsonge Please confirm to revert #30707.

avatar brianteeman
brianteeman - comment - 20 May 2021

reverting that PR will not achieve that

avatar Quy
Quy - comment - 20 May 2021

Then I am lost what the solution should be.

avatar brianteeman
brianteeman - comment - 20 May 2021

The solution is not to do anything

avatar Quy
Quy - comment - 20 May 2021

So it is ok to have mixed icon- and fa- in the backend?

avatar brianteeman
brianteeman - comment - 20 May 2021

yes - I see no reason why not. Especially for the utility classes

avatar Quy
Quy - comment - 20 May 2021

Closing then. Thanks!

avatar Quy Quy - close - 20 May 2021
avatar Quy Quy - change - 20 May 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-05-20 17:17:43
Closed_By Quy
Labels Added: NPM Resource Changed ?

Add a Comment

Login with GitHub to post a comment