Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(css): rename $prefix in $func-prefix in get-color-from-rgba-string function #1567

Merged
merged 1 commit into from
Oct 24, 2022

Conversation

julien-deramond
Copy link
Member

Description

Issue reported by @Lausselloic with the following project: demo-custom-build.zip

Download the zip file and run unzip -d demo-custom-build demo-custom-build.zip && cd demo-custom-build/demo-custom-build && npm i && npm run css

Error:

CssSyntaxError: /Users/ju/Downloads/demo-custom-build/node_modules/boosted/scss/mixins/_utilities.scss:82:16: Unclosed bracket

  80 |             @if $is-local-vars {
  81 |               @each $local-var, $variable in $is-local-vars {
> 82 |                 --#{$prefix}#{$local-var}: #{$variable};
     |                ^
  83 |               }
  84 |             }

Now open node_modules/boosted/scss/mixins/_utilities.scss and modify it to have the same content as in this PR.
If you re-run npm run css the compilation is now OK.

⚠️ Dear reviewer, please check that this modification has no impact in a normal usage regarding the usage of get-color-from-rgba-string.

Motivation & Context

For security, IMO we should apply this patch to avoid unexpected results when having 2 $prefix definitions.
However, I don't understand for now the difference between this build and a fresh build with Vite for example where the issue doesn't seem to appear. It must be linked to:

.od-boosted-theme {
    @import "../../node_modules/boosted/scss/boosted";
}

Types of change

  • Bug fix (non-breaking which fixes an issues)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • All new and existing tests passed

@julien-deramond julien-deramond added this to Triage in v5.2.1 via automation Oct 21, 2022
@julien-deramond julien-deramond moved this from Triage to In progress in v5.2.1 Oct 21, 2022
@netlify
Copy link

netlify bot commented Oct 21, 2022

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit e627a21
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/6352911d4fb8580009e8dde0
😎 Deploy Preview https://deploy-preview-1567--boosted.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@sonarcloud
Copy link

sonarcloud bot commented Oct 21, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@MewenLeHo MewenLeHo self-assigned this Oct 21, 2022
Copy link
Contributor

@MewenLeHo MewenLeHo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No regression spotted so ok for me 🟢

@julien-deramond julien-deramond merged commit d44ea6a into main Oct 24, 2022
@julien-deramond julien-deramond deleted the main-jd-change-prefix-var-name branch October 24, 2022 05:48
@julien-deramond julien-deramond moved this from In progress to Done in v5.2.1 Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants