Upgrade Font Awesome v4 => v6

[ufundo]

Overview

Attempt to upgrade the core version of FontAwesome from v4 to v6. This came out of discussions on Riverlea theme work. However due to the integration with the iconpicker, I think it doesn't make sense to try to tackle icon set at theme level, so have tried here.

Before

CiviCRM core packages FontAwesome 4 (2017 vintage)

After

FontAwesome 6 Free, with compatibility shims for v4.

(duplicates now removed, thanks @colemanw )

Existing use of v4 classes should still display, thanks to shims provided by FA.

The icon picker gallery is sourced from the classes in the main css file. This means it shows only v6 classes. When editing an icon that was previously v4 user will be forced to choose a v6 class.

Technical Details

The shim should be namespaced to crm-i. However I'm not sure how/if I have carried over equivalent namespacing to the v6 loading.

Comments

Initial discussion: https://lab.civicrm.org/extensions/riverlea/-/issues/28

I'm not too hot on the licensing details, but appears to be CC BY 4.0 - which I think is ok?

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[ufundo]

@vingle found this issue with the placeholder font going Comic Sans--y if an input has mixed icon / text placeholder.

[ufundo]

@vingle found this issue with the placeholder font going Comic Sans--y if an input has mixed icon / text placeholder.

Fixed ! Thanks @vingle

[colemanw]

@ufundo test failure points to our policy of not using minified files. Can you use the non-minified shims.css instead?

[colemanw]

@ufundo the home dashboard looks like this on the demo site:

[ufundo]

Hmm, test site was working for me last night so that's a step back.

I thought I had bundled the shim twice but maybe both calls were needed after all.

Can switch to non-minified yes 👍

[colemanw]

Love the new icons & very cool it all works with the icon picker!

[colemanw]

Can switch to non-minified yes 👍

Now I see the previous files were minified, so I'm a bit confused by the test fail... but perhaps the difference is that there needs to also be a non-minified version present?

[ufundo]

I think it doesnt mind minified in bower_components and that the issue is the minified crm-i'ified shim in css

[ufundo]

thanks for looking @colemanw - glad you like the spaghetti codes 🍝 😁

home dashboard icons: these weren't loading due to a font-weight: normal; declaration on them. removing that fixes them... but it does leave open whether that might break other places. I'm not sure why it doesn't seem to load the new Font Awesome 6 Free font at that weight. @vingle ?

minified complaint: tried just adding the un-minified alongside but it still complained, have removed the unminified now

** iconpicker issue ** : found an issue that if you had an existing icon that was v4 only, it wouldn't show the current value when you opened the icon picker. adding 'crm-i' to bring in the shims seems to have fixed.

** iconpicker styles **: I've taken these out of the jQuery and dropped in the crm-i.css file. It's a few lines of css you're loading when you often don't need it, but css dropped in by jQuery is quite icky to me. And I think they could do with being cleaned up. I've resisted trying to clean for now though.

[colemanw]

It's a few lines of css you're loading when you often don't need it, but css dropped in by jQuery is quite icky to me. And I think they could do with being cleaned up. I've resisted trying to clean for now though.

Yea that's fine for now. Could move them into a separate stylesheet that loads with the iconpicker, but not a blocker.

[colemanw]

@ufundo style checker says "Missing newline at the end of crm-i-v4-shims.css"

[ufundo]

Fixed. Also realised icon picker regex wasn't pulling in the brand icons, because those are declared slightly differently - so now even more icons..

[ufundo]

The v4 shim is non-negligible. I think a good next step could be to make it optional so people can disable if they don't need it any more.

[ufundo]

hmm, brands loaded ok for me 🤔

[vingle]

Side-issue, kinda small, but this has been bugging me since I looked at it yesterday. v4-shims seems more verbose than it needs to be - and is 42.5kb,(1/4 the size of RiverLea). The minification removes a bit of white space but doesn't remove/merge the 318 selectors that say, over and over, like they've never seen a comma:

.crm-i.fa-something {
  font-family: 'Font Awesome 6 Free';
  font-weight: 400;
}
.crm-i.fa-or-other {
  font-family: 'Font Awesome 6 Free';
  font-weight: 400;
}

At first I thought group them but .crm-i is already pointing to the font-family, so I think they could be grouped together to declare just the weight (which is different to the rest of crm-i) and then reduce the file size by I'm guessing 1/3 to a 1/4.

[colemanw]

@ufundo I flushed caches again and now they're ok 👍🏻

[colemanw]

@vingle I think they could be grouped together to declare just the weight (which is different to the rest of crm-i) and then reduce the file size by I'm guessing 1/3 to a 1/4.

That's a good idea, although I think the ultimate goal will be to convert all our core markup to the v6 classes and then we can eventually remove it.
Although it will probably need a several-years-long transition period before we can do so.

@ufundo The v4 shim is non-negligible. I think a good next step could be to make it optional so people can disable if they don't need it any more.

I would be 👎🏻 on adding a setting to make it optional, because "yet another setting®"

[colemanw]

@ufundo IMO this is good to go.

[vingle]

That's a good idea, although I think the ultimate goal will be to convert all our core markup to the v6 classes and then we can eventually remove it.

Will trim it down today - I'm guessing it will take a while for extensions to update their classes too.

[ufundo]

@vingle - I know what you mean... I guess it is just geared towards readability but could be good to trim it.

@colemanw

I would be 👎🏻 on adding a setting to make it optional, because "yet another setting®"

Fair. I suppose I was thinking it could help transition: if we stripped all the v4 icons from core, we could turn it off (with a way for extensions to indicate they need it?). So something low level behind-the-scenes rather than "here's something else you have to decide when setting up your system".

[ufundo]

I'm going to mark Ready for Review but I think it would be good to get some wider testing of potential knock-on effects in more complicated set ups!

[vingle]

Have PRd to your branch - halved the file size: ufundo#4

[ufundo]

Thanks @vingle ... let's see what our linter overlords make of it.... 👀

[ufundo]

Afraid it doesn't like it @vingle 😬

civilint: civicrm-core#30779

Executed circa 2024-08-01 04:38 -07:00

===========================[ Identify Files ]===========================
PHP Files:
 * CRM/Core/Resources/Common.php
 * Civi/Standalone/WebEntrypoint.php
Javascript Files:
 * js/jquery/jquery.crmIconPicker.js
CSS Files:
 * css/crm-i-v4-shims.css
 * css/crm-i.css
 * css/crm-menubar.css
 * css/dashboard.css
Skip Files:
 * composer.json
 * tools/standalone/router.php
===============================[ php -l ]===============================
===============================[ phpcs  ]===============================
=============================[ phpcs (css) ]============================

FILE: ...home/homer/buildkit/build/build-0/web/src/css/crm-i-v4-shims.css
----------------------------------------------------------------------
FOUND 934 ERRORS AFFECTING 312 LINES
----------------------------------------------------------------------
   8 | ERROR | [x] Multiple selectors should each be on a single line
   8 | ERROR | [x] Multiple selectors should each be on a single line
   8 | ERROR | [x] Multiple selectors should each be on a single line
   8 | ERROR | [x] Multiple selectors should each be on a single line
   8 | ERROR | [x] Multiple selectors should each be on a single line
   8 | ERROR | [x] Multiple selectors should each be on a single line
   8 | ERROR | [x] Multiple selectors should each be on a single line
   8 | ERROR | [x] Multiple selectors should each be on a single line
   8 | ERROR | [x] Multiple selectors should each be on a single line
...

[ufundo]

Trying again with just the grouping (but leaving in the carriage returns)

[colemanw]

@ufundo I just pushed some improvements to the iconPicker & moved the stylesheet.

[colemanw]

Everything works in my testing. I'm merging this & it can get tested by others in the RC.