Conversation
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
@crhallberg, I see that some tests are failing here -- is that a problem, or do the tests themselves need adjustment?
|
Do we want to target this to |
We can wait and see what @EreMaijala thinks... |
|
@crhallberg I think it might be better to leave bootstrap3 as is. Otherwise it just adds a bit more maintenance burden for anyone upgrading and still using bootstrap3 as their base theme. It's not a huge deal though, but I'm just thinking that all the theme-related changes already add quite a bit of work. |
|
That's a good point, @EreMaijala -- if we leave this until 11.0, then bootstrap3 will be deleted anyway and it will be less to deal with. :-) What do you think, @crhallberg? Are there changes here that are actively fixing a problem in the bootstrap5 theme, or is this more preventative? |
|
@demiankatz This is fixing some errors with the icon, since the classes are different from FA4 to FA6 (which BS5 uses). I used it as an excuse to create these mixins. If I was going to check every icon by hand, I may as well refactor, right? 😅 |
In that case, would the solution be to create a PR that just changes the existing icon configuration in the bootstrap5 theme to incorporate your fixes (which I assume can be copied and pasted from the mixin here) and target that against 10.1? Then we don't have major refactoring at the last minute before the release but we can benefit immediately from your research... and this work can be developed in a more leisurely way as we move toward 11.0. :-) |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
@crhallberg, a couple of issues here:
1.) Looks like some tests are failing and/or throwing "no assertions" warnings. Is this on your radar?
2.) The very large number of changed/added files in this PR is causing GitHub to struggle, which makes reviewing difficult. Can we adapt the npm-based vendor file loading process from the themes to the mixins and then only copy over the pieces that are essential? I'm hoping that we don't actually need thousands of individual svgs. If this file load is unavoidable, maybe it's an argument for moving the non-default mixins to a separate repo so they can be optionally included without adding so much weight to the project.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @crhallberg! See below for the latest round of comments.
| tests/phpcs.cache.json | ||
| themes/bootstrap5/scss/vendor/bootstrap | ||
| themes/mixin-icons-fontawesome-6/css/vendor | ||
| themes/mixin-icons-fontawesome-7/css/vendor |
There was a problem hiding this comment.
Is .gitignore the only way to avoid thousands of files in these vendor directories? If so, this intersects with the discussions on #4591 about our general strategy for JS-adjacent dependencies. If not, can we just commit the smaller subset of needed files and not .gitignore vendor files at this time?
There was a problem hiding this comment.
I can remove this right before we merge so that all the files are in place.
| if (str_contains($icon, ':')) { | ||
| $parts = explode(':', $icon, 2); | ||
| $icon = $parts[0]; | ||
| $class = str_replace('.', ' ', $parts[1]); |
There was a problem hiding this comment.
What is the purpose of this string replace? Is it worth adding a comment?
themes/bootstrap5/templates/RecordDriver/DefaultRecord/explain-line.phtml
Show resolved
Hide resolved
| @@ -0,0 +1 @@ | |||
| css/vendor/fontawesome-free-6 | |||
There was a problem hiding this comment.
I don't think we need both top-level and mixin-level .gitignore files, if we need .gitignore at all (as discussed above).
| <?php | ||
| $class = trim('icon icon--text ' . ($this->extra['class'] ?? '')); | ||
| ?> | ||
| <span class="<?=$this->escapeHtmlAttr($class) ?>"<?=$this->attrs ?> role="img" aria-hidden="true"><?=$this->escapeHtmlAttr($this->icon)?></span> |
There was a problem hiding this comment.
Why are we using escapeHtmlAttr on the icon when it is outside of an HTML attribute?
| $class = trim('icon icon--text icon--unicode ' . ($this->extra['class'] ?? '')); | ||
| ?> | ||
| <span class="<?=$this->escapeHtmlAttr($className) ?>"<?=$this->attrs ?> role="img" aria-hidden="true" data-icon="&#x<?=$this->escapeHtmlAttr($this->icon)?>;"></span> | ||
| <span class="<?=$this->escapeHtmlAttr($class) ?>"<?=$this->attrs ?> role="img" aria-hidden="true">&#x<?=$this->escapeHtmlAttr($this->icon)?>;</span> |
There was a problem hiding this comment.
Another escapeHtmlAttr outside of an attribute.
|
I've opened #4789 and reverted some minor changes here to make it easier to do side-by-side comparison of the new icons proposed here vs. the existing icons currently in the dev branch. The only thing that is currently preventing the icon lists from aligning correctly is the introduction of the right-to-left recall icon here; I wonder if we should reconsider that choice for easier comparison (and because I'm not sure if it's the most appropriate icon anyway). |
|
I've also opened #4790 to better display right-to-left icons in the dev tool; this will improve side-by-side comparisons even if we do not revert the recall changes mentioned in my previous comment. |
3 participants
This is a fix for a few Font Awesome 6 icons spun out of control. If this is too much too soon, I can do a smaller version of this for v10.1 but editing
bootstrap5/theme.config.php.Just Before Merge