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

Vendor in Sass-MQ #601

Merged
merged 3 commits into from
Jan 18, 2021
Merged

Vendor in Sass-MQ #601

merged 3 commits into from
Jan 18, 2021

Conversation

pezholio
Copy link
Contributor

@pezholio pezholio commented Apr 6, 2020

Description

We're trying to use NHSUK Frontend on a project which has a non-standard location for node_modules, and as a result, we were getting the following error:

sass.CompileError: Error: File to import not found or unreadable: ./node_modules/sass-mq/mq.
        on line 17:1 of assets/_dev/node_modules/nhsuk-frontend/packages/core/tools/_sass-mq.scss
        from line 9:1 of assets/_dev/node_modules/nhsuk-frontend/packages/core/tools/_all.scss
        from line 6:1 of assets/_dev/scss/screen.scss
>> @import './node_modules/sass-mq/mq'; // [1] //

After a bit of Googling, I found that govuk-frontend had a similar problem and solved this by vendoring in SassMQ. I've attempted to replicate this approach in NHS Frontend too.

Thoughts welcome!

Checklist

@chrimesdev
Copy link
Member

chrimesdev commented Apr 6, 2020

Hey @pezholio thanks for looking at this, seems like a sensible change to me. I'll get a second pair of eyes on this but looks good 👍

@mikemonteith
Copy link
Contributor

Should we be using an includePath setting instead, and then have this in our scss:

@import 'sass-mq/mq';

https://sass-lang.com/documentation/at-rules/import#load-paths

@mikemonteith
Copy link
Contributor

Should we be using an includePath setting instead, and then have this in our scss:

@import 'sass-mq/mq';

https://sass-lang.com/documentation/at-rules/import#load-paths

This would require all users of this library to also have an includePath in their build settings.

@pezholio
Copy link
Contributor Author

Hey, any news on this? We'd really like to get back using the main version of NHSUK frontend, but this is blocking us

@chrimesdev
Copy link
Member

Happy to stick with the approach in the PR as this is consistent with GOVUK frontend.

@pezholio please could you rebase this with master to be released in 4.1.0 and I can get this merged.

@chrimesdev chrimesdev added this to the 4.1.0 milestone Jan 15, 2021
@pezholio
Copy link
Contributor Author

@AdamChrimes All up to date and rebased! Lemme know if there's anything not quite right 👍

@chrimesdev chrimesdev changed the base branch from master to dev January 18, 2021 19:29
@chrimesdev chrimesdev merged commit 11f459b into nhsuk:dev Jan 18, 2021
@chrimesdev
Copy link
Member

chrimesdev commented Jan 18, 2021

Thanks @pezholio will get this released as part of v4.1.0 hopefully this week (26th at the very latest).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants