Problem/Motivation
We need icons font generator. Specification:
- Support font icons generator with current drush theme generator. The generator will copy all files to child theme that needs to support child theme font generation - all icons, all build files, etc. If some of the files exist on the target (SVG icons) they will not be overridden.
- Add npm command for font generation.
- Reuse/clone process for bootstrap font icon generation. Use https://github.com/twbs/fantasticon which is a fork of https://github.com/tancredi/fantasticon project.
- Each theme will get a new images/font-icons folder where it can put SVG icons for font generation.
- Add a library that loads font icons and related CSS code.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | font-icon-refactoring-3351555-17.patch | 106.65 KB | pivica |
| #17 | interdiff-3351555-16-17.txt | 4.88 KB | pivica |
Comments
Comment #2
pivica commentedHere is a first patch, quite a complex thing to implement in order to ensure child themes updating workflow and respecting old child themes that will not want to switch to new icons.
Now lets do some testing.
@TODO - we need to write documentations for this before closing this issue!
Comment #3
pivica commentedSome minor refactoring.
Comment #4
pivica commentedI am having a trouble applying previous patch with composer because of binary icons.woff2 file which I used --binary flag. Not sure why, and no time to investigate, here is a patch without icons.woff2 - remember to generate it before committing this.
Comment #5
pivica commentedOne problem I just figured out is that fantasticon is not centering icons vertically inside of the font, more about this in this issue . We need to take this into consideration when generating our fonts. It seems that descent option with value 55 is giving us a good result for icons from free Fontawesome 6 set. Adding this and code comments that explains this,
Comment #6
pivica commentedFigured out we need to add arrow-up and close icons which we use in bs_bootstrap theme.
Comment #7
pivica commentedRemoved extra space.
Comment #8
pivica commentedDraft documentation added on https://www.drupal.org/docs/8/themes/bs-base/additional-features/icons-f....
Comment #9
primsi commentedReviewed just a few initial files.
Not sure if drush_log is still supported.
We could use the FileSystem helper here and below that provides tools to work with the file system. FileSystem::prepareDirectory for example checks this and creates if it doesn't exist.
Do we need to skip hidden files as well? Might be good for some OS-es.
Comment #10
pivica commentedUpdated patch against latest beta5.
Comment #11
pivica commented> Not sure if drush_log is still supported.
Yep that should be fixed. I also figured that we need to fix drush_print which is also depreciated. I also did a bunch of small php code optimization that phpstorm suggested. This changes could go to different issue, but I am lazy to create a new one, let's keep it here for now.
We also need to change drush_confirm and drush_user_abort which we use in _bs_base_theme_run_update_hooks(). But we can't do this for now, first we need to move all this code to bs_lib/src/Commands/BsLibCommands.php and then we can replace this calls with $this->io()->confirm() and UserAbortException() as shown in https://github.com/drush-ops/drush/commit/277ccb6d19ed6d85f372780cff4ea1.... We have #3107100: Move all related generator code to bs_lib from bs_base for this and it seems now it is a time to do it finally.
> We could use the FileSystem helper here and below that provides tools to work with the file system. FileSystem::prepareDirectory for example checks this and creates if it doesn't exist.
I am not sure are we allowed to use Drupal::service('file_system') call here because we need this code to work also without Drupal instance. Will check with @Berdir.
> Do we need to skip hidden files as well? Might be good for some OS-es.
No idea for now.
Comment #12
pivica commented> I am not sure are we allowed to use Drupal::service('file_system') call here because we need this code to work also without Drupal instance. Will check with @Berdir.
Checked this, I think we could use Drupal::service('file_system') however current logic is not that simple and replacing all this calls with Drupal file_system would require a bit of not that trivial work and testing then that everything works as expected. I do not have enough time to do this now, created a follow-up for this in #3380488: Consider using Drupal file_system API instead of PHP file calls for the future when/if makes sense.
Comment #13
pivica commentedAfter a ton of testing, code checking, bug fixing... finally a new patch version with next changes:
- Fixed problem with compiling unicode hex values with SASS.
- Updated SVG icons for Font Awsome 6.4.2
- Added $bs-icon-shims SASS variable for shim CSS rules generation.
- Improved font hash busting URL generation in SASS.
- Updated fantasticon package to 2.7.1
- Update function will add variables/icons import to theme SASS init.
- Generator will add variables/icons import to theme SASS init.
- Removed automatic font icons preloading, better to leave that to child theme.
- New fonts are disabled by default in theme settings.
Do not forget to generate fonts/icons.woff2 when committing this.
Comment #14
pivica commentedI think there is no need for `fa-ext` shim, recompiled CSS.
Comment #15
pivica commented@Berdir suggested to improve text in theme setting admin UI for this new option.
Comment #16
pivica commentedDiscussed with @Berdir and he suggested that having two options (on SASS level and Drupal theme settings level) is too much and that it would be better to have only one option that we should change for projects that wants to use new icons system. We figured out next changes:
1. We will leave $bs-icon-use SASS variable and if it is set to false then icons.css should be empty. Meaning there is no library any more, we always load this for a theme.
It is fine then to use the same SASS variable to decide to override fa-icon() or not so we are compatible with old projects.
2. And for the Drupal PHP part, we will remove bs_bootstrap.use_icons.
NOTE that bs_bootstrap it self is not loading FA4 resource. If your custom themes are loading FA4 you will need to figure a way to detect if `$bs-icon-use` is set to true in SASS and then do not load old FA4 resource. In Primer we solved this by checking `theme-options.yml` for a presence of `bs-icon-use: true` string.
Here is a new patch.
Comment #17
pivica commentedMinor improvement, added new helper function bs-icon-content(), using generator in client project suggest that this is a good idea.
Comment #19
pivica commentedCommitted.