- Issue created by @pivica
- 🇷🇸Serbia pivica
Here 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!
- Status changed to Needs review
over 1 year ago 1:22pm 31 March 2023 - 🇷🇸Serbia pivica
I 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.
- 🇷🇸Serbia pivica
One 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,
- 🇷🇸Serbia pivica
Figured out we need to add arrow-up and close icons which we use in bs_bootstrap theme.
- 🇷🇸Serbia pivica
Draft documentation added on https://www.drupal.org/docs/8/themes/bs-base/additional-features/icons-f... → .
- Status changed to Needs work
over 1 year ago 3:01pm 24 May 2023 - 🇸🇮Slovenia primsi
Reviewed just a few initial files.
-
+++ b/bs_base.drush.inc @@ -238,7 +238,7 @@ function drush_bs_base_bs_theme_update($target_machine_name) { + drush_log("Building assets for a $theme_machine_name theme", LogLevel::INFO);
Not sure if drush_log is still supported.
-
+++ b/bs_base.drush.inc @@ -414,6 +463,62 @@ function _bs_base_copy_file($file, array $options) { + if (!is_dir($destination)) {
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.
-
+++ b/bs_base.drush.inc @@ -414,6 +463,62 @@ function _bs_base_copy_file($file, array $options) { + // Skip special files "." and ".."
Do we need to skip hidden files as well? Might be good for some OS-es.
-
- Status changed to Needs review
over 1 year ago 5:54pm 20 June 2023 - 🇷🇸Serbia pivica
> 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.
- 🇷🇸Serbia pivica
> 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 📌 Consider using Drupal file_system API instead of PHP file calls Active for the future when/if makes sense.
- 🇷🇸Serbia pivica
After 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.
- 🇷🇸Serbia pivica
I think there is no need for `fa-ext` shim, recompiled CSS.
- 🇷🇸Serbia pivica
@Berdir suggested to improve text in theme setting admin UI for this new option.
- 🇷🇸Serbia pivica
Discussed 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.
- 🇷🇸Serbia pivica
Minor improvement, added new helper function bs-icon-content(), using generator in client project suggest that this is a good idea.
- Status changed to Fixed
about 1 year ago 5:43pm 25 September 2023 Automatically closed - issue fixed for 2 weeks with no activity.