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

Interpolate sass variables inside calc() to maintain support for compiling with libsass #4782

Closed
oscarduignan opened this issue Feb 20, 2024 · 0 comments · Fixed by #4784
Closed
Assignees
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation)
Milestone

Comments

@oscarduignan
Copy link
Contributor

Description of the issue

In 5.1.0 you introduced some styles that mean that your sass is no longer compatible with libsass due to lack of interpolation within some uses of "calc()"

Steps to reproduce the issue

  1. grab the latest from main
  2. cat dist/*.css | npx prettier --stdin-filepath .css | grep "calc(100% -"
  3. you will see this:
    max-width: calc(100% - 74px);
    max-width: calc(100% - 74px);
    
  4. npm install node-sass
  5. cd src/govuk
  6. npx node-sass all.scss | grep "calc(100% -"
  7. and now you will see:
    max-width: calc(100% - (($govuk-checkboxes-label-padding-left-right * 2) + $govuk-touch-target-size));
    max-width: calc(100% - ($govuk-radios-label-padding-left-right + $govuk-touch-target-size + govuk-spacing(3)));
    

because the contents of calc() are treated as an unquoted string in libsass, it outputs everything literally

in hmrc-frontend I think we must have something that's throwing an exception on this - possibly stylelint or something like this when we build - but node-sass won't through an error itself, it will just output it literally, so it's possible that 5.1.0 might break some people's usages of govuk-frontend unknowingly if they are using node-sass to compile your scss without any other checks that it generated valid css

Actual vs expected behaviour

libsass is deprecated but you could maintain compatibility with it by interpolating the sass variables like this:

   max-width: calc(100% - #{(($govuk-checkboxes-label-padding-left-right * 2) + $govuk-touch-target-size)});
   max-width: calc(100% - #{($govuk-radios-label-padding-left-right + $govuk-touch-target-size + govuk-spacing(3))});

the before and after output with dart-sass and libsass is:

   max-width: calc(100% - 74px);
   max-width: calc(100% - 74px);

(for an example see this branch in a fork)

Environment (where applicable)

we're using node-sass@7 (sorry) but I tried updating hmrc-frontend through all the newer major versions (8 and 9) and still got the issue with compilation.

  • Operating system: N/A
  • Browser: N/A
  • Browser version: N/A
  • GOV.UK Frontend Version: 5.1.0
@oscarduignan oscarduignan added awaiting triage Needs triaging by team 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) labels Feb 20, 2024
@36degrees 36degrees moved this to Backlog 🏃🏼‍♀️ in GOV.UK Design System cycle board Feb 20, 2024
@36degrees 36degrees added this to the v5.2 milestone Feb 20, 2024
colinrotherham added a commit that referenced this issue Feb 20, 2024
See GitHub issue: #4782

Co-authored-by: Oscar Duignan <100650+oscarduignan@users.noreply.github.com>
@colinrotherham colinrotherham moved this from Backlog 🏃🏼‍♀️ to Needs review 🔍 in GOV.UK Design System cycle board Feb 20, 2024
@colinrotherham colinrotherham removed the awaiting triage Needs triaging by team label Feb 20, 2024
@36degrees 36degrees moved this from Needs review 🔍 to Done 🏁 in GOV.UK Design System cycle board Feb 22, 2024
owenatgov pushed a commit that referenced this issue Apr 4, 2024
See GitHub issue: #4782

Co-authored-by: Oscar Duignan <100650+oscarduignan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation)
Projects
Development

Successfully merging a pull request may close this issue.

3 participants