Fixed scss files to show icons correctly #16
Conversation
Summary of ChangesHello @haanjack, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on modernizing and fixing the styling logic for icons within the project. It updates Sass module imports and function calls to align with current Sass best practices, and specifically addresses an issue where Font Awesome icons might not have been rendering correctly due to improper string unquoting. The changes ensure consistent and correct display of icons across the application by updating syntax and removing redundancies. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the SCSS files from the modern Sass module system (@use/@forward) to the older @import rule, likely to resolve compatibility issues with your build environment. While most of the changes correctly adapt to the new import system, I've identified a critical issue in several font-awesome files where the use of unquote() will lead to invalid CSS and broken icons. My review includes suggestions to fix this, ensuring the icons render correctly as intended.
| #{$fa-icon-property}: unquote("\"#{ $icon }\""); | ||
| #{$fa-duotone-icon-property}: unquote("\"#{$icon}#{$icon}\""); |
There was a problem hiding this comment.
The use of unquote() here will produce an invalid value for the CSS custom properties. For the content property to correctly render the icon, the custom property's value must be a string literal containing the unicode escape sequence. The current implementation generates an unquoted value, which will break icon rendering.
| #{$fa-icon-property}: unquote("\"#{ $icon }\""); | |
| #{$fa-duotone-icon-property}: unquote("\"#{$icon}#{$icon}\""); | |
| #{$fa-icon-property}: "#{$icon}"; | |
| #{$fa-duotone-icon-property}: "#{$icon}#{$icon}"; |
| & { #{$fa-icon-property}: unquote("\"#{ $fa-var }\""); #{$fa-duotone-icon-property}: unquote("\"#{ $fa-var }#{ $fa-var }\""); } | ||
| } | ||
| @mixin fa-icon-regular($fa-var) { | ||
| @extend .fa-regular; | ||
|
|
||
| & { #{$fa-icon-property}: string.unquote("#{ $fa-var }"); #{$fa-duotone-icon-property}: string.unquote("#{ $fa-var }#{ $fa-var }"); } | ||
| & { #{$fa-icon-property}: unquote("\"#{ $fa-var }\""); #{$fa-duotone-icon-property}: unquote("\"#{ $fa-var }#{ $fa-var }\""); } | ||
| } | ||
| @mixin fa-icon-brands($fa-var) { | ||
| @extend .fa-brands; | ||
|
|
||
| & { #{$fa-icon-property}: string.unquote("#{ $fa-var }"); #{$fa-duotone-icon-property}: string.unquote("#{ $fa-var }#{ $fa-var }"); } | ||
| } | ||
| & { #{$fa-icon-property}: unquote("\"#{ $fa-var }\""); #{$fa-duotone-icon-property}: unquote("\"#{ $fa-var }#{ $fa-var }\""); } |
There was a problem hiding this comment.
Similar to the issue in _icons.scss, the use of unquote() in these mixins will produce invalid values for the CSS custom properties. The value should be a string literal for the content property to work correctly. This change should be applied to all three mixins (fa-icon-solid, fa-icon-regular, fa-icon-brands) in this file.
| & { #{$fa-icon-property}: unquote("\"#{ $fa-var }\""); #{$fa-duotone-icon-property}: unquote("\"#{ $fa-var }#{ $fa-var }\""); } | |
| } | |
| @mixin fa-icon-regular($fa-var) { | |
| @extend .fa-regular; | |
| & { #{$fa-icon-property}: string.unquote("#{ $fa-var }"); #{$fa-duotone-icon-property}: string.unquote("#{ $fa-var }#{ $fa-var }"); } | |
| & { #{$fa-icon-property}: unquote("\"#{ $fa-var }\""); #{$fa-duotone-icon-property}: unquote("\"#{ $fa-var }#{ $fa-var }\""); } | |
| } | |
| @mixin fa-icon-brands($fa-var) { | |
| @extend .fa-brands; | |
| & { #{$fa-icon-property}: string.unquote("#{ $fa-var }"); #{$fa-duotone-icon-property}: string.unquote("#{ $fa-var }#{ $fa-var }"); } | |
| } | |
| \ No newline at end of file | |
| & { #{$fa-icon-property}: unquote("\"#{ $fa-var }\""); #{$fa-duotone-icon-property}: unquote("\"#{ $fa-var }#{ $fa-var }\""); } | |
| & { #{$fa-icon-property}: "#{$fa-var}"; #{$fa-duotone-icon-property}: "#{$fa-var}#{$fa-var}"; } | |
| } | |
| @mixin fa-icon-regular($fa-var) { | |
| @extend .fa-regular; | |
| & { #{$fa-icon-property}: "#{$fa-var}"; #{$fa-duotone-icon-property}: "#{$fa-var}#{$fa-var}"; } | |
| } | |
| @mixin fa-icon-brands($fa-var) { | |
| @extend .fa-brands; | |
| & { #{$fa-icon-property}: "#{$fa-var}"; #{$fa-duotone-icon-property}: "#{$fa-var}#{$fa-var}"; } |
| @each $name, $icon in $fa-brand-icons { | ||
| .#{$fa-css-prefix}-#{$name} { #{$fa-icon-property}: string.unquote("#{ $icon }"); } | ||
| } | ||
| .#{$fa-css-prefix}-#{$name} { #{$fa-icon-property}: unquote("\"#{ $icon }\""); } |
There was a problem hiding this comment.
The use of unquote() here will produce an invalid value for the CSS custom property. For the icon to render correctly, the value assigned to the custom property must be a string literal containing the unicode escape sequence.
| .#{$fa-css-prefix}-#{$name} { #{$fa-icon-property}: unquote("\"#{ $icon }\""); } | |
| .#{$fa-css-prefix}-#{$name} { #{$fa-icon-property}: "#{$icon}"; } |
| @import | ||
| "variables", | ||
| "themes", | ||
| "layout", | ||
| "base", | ||
| "distill", | ||
| "cv", | ||
| "tabs", | ||
| "typograms", | ||
| "font-awesome/fontawesome", | ||
| "font-awesome/brands", | ||
| "font-awesome/solid", | ||
| "font-awesome/regular", | ||
| "tabler-icons/tabler-icons.scss", | ||
| "tabler-icons/tabler-icons-filled.scss", | ||
| "tabler-icons/tabler-icons-outline.scss" | ||
| ; |
There was a problem hiding this comment.
While migrating from @use to @import might be necessary for your build environment, it's worth noting that @import is being deprecated in Sass. For better maintainability and to avoid global scope pollution, consider using the modern module system with @use and @forward if your build tools support it in the future. This change to a single @import statement is a good way to manage dependencies with the older system.
No description provided.