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

Allow for a single-file import of all elements stylesheets #489

Merged
merged 3 commits into from
Jun 8, 2017

Conversation

lennym
Copy link
Contributor

@lennym lennym commented Jun 7, 2017

Move the import of the govuk-elements styles into a single, standalone file so they can be imported as a single import statement without also including govuk_frontent_toolkit files.

What problem does the pull request solve?

We use a sass compiler which is npm-aware (https://npmjs.com/npm-sass), and so generally don't use configured includePaths when compiling. However, the main sass file in this repo is only compilable if the includePaths are set.

By moving the govuk-elements-sass code (as distinct from the govuk_frontend_toolkit code), we can @import "govuk-elements-sass" in our stylesheets and have them compile correctly (note the sass property added to package.json supports this shorthand, as opposed to having to specify a full file path)

This will also support any users who wish to import the elements code alone, without also importing govuk_frontend_toolkit for whatever reason.

What type of change is it?

  • Bug fix (non-breaking change which fixes an issue)

Has the documentation been updated?

  • I have read the CONTRIBUTING document.

@gemmaleigh
Copy link
Contributor

Thanks @lennym!

The relationship between the govuk-elements-sass files and the govuk_frontend_toolkit isn't explicit (ideally the imports would be at the top of each file - not that it would resolve the issue with includePaths - but at least the relationship between the two would be clearer.)

I've been trying to avoid making breaking changes - which is why they're still listed at the top.

This looks good, I'm hoping to release a new version of govuk-elements-sass shortly and will include this in that release.

@lennym
Copy link
Contributor Author

lennym commented Jun 8, 2017

Thanks a lot @gemmaleigh.

One thing I might suggest, that would be super helpful to us at least, (and since you seem to have ownership of govuk-frontend-toolkit too) would be to have all those dependencies import-able as a single file - something like this PR - rather than needing to pull them all in individually.

@gemmaleigh
Copy link
Contributor

gemmaleigh commented Jun 8, 2017

I won't be able to merge this PR as-is, as the package is built from assets in assets/, which is where the files will need splitting. Are you happy to do this?

@lennym
Copy link
Contributor Author

lennym commented Jun 8, 2017

I think I see what you mean... the same set of changes should be made to the files in the assets directory in the repo root?

Will do. Give me 5 minutes.

@gemmaleigh
Copy link
Contributor

gemmaleigh commented Jun 8, 2017

You'll only need to make the changes once in the assets directory, there's a gulp package task which I'll run before publishing the package, that'll copy any changes to the scss files to /packages/.

https://github.com/alphagov/govuk_elements/blob/master/gulpfile.js#L127

Cheers!

@lennym
Copy link
Contributor Author

lennym commented Jun 8, 2017

I've pushed the changes in assets - actually an extra change to also group the govuk-frontend-toolkit files as well, as IMO this makes it a lot clearer what is coming from where.

If you want me to undo the changes in packages then I'll rebase -i them out.


Edit: looks like I'll need to add the extra files to the gulp task too.

@gemmaleigh
Copy link
Contributor

If you could undo the changes in packages (aside from the change to the package.json file) and ensure those two new files are copied over, that would be great - thanks!

lennym added 3 commits June 8, 2017 16:52
Makes it slightly clearer when reading the main sass entry point file which of the modules are coming from govuk-frontent-toolkit, and which are coming from this module.
This allows sass compilers which dynamically look for a module's entry-point file to read the correct file.
@lennym lennym force-pushed the feature/single-elements-file branch from 072fe95 to 93f4448 Compare June 8, 2017 15:58
@lennym
Copy link
Contributor Author

lennym commented Jun 8, 2017

Done. Let me know if I need to do anything else.

Copy link
Contributor

@gemmaleigh gemmaleigh left a comment

Choose a reason for hiding this comment

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

This works nicely, thanks for making those changes - much appreciated!

@gemmaleigh gemmaleigh merged commit 98faa99 into alphagov:master Jun 8, 2017
@lennym lennym deleted the feature/single-elements-file branch June 8, 2017 16:21
gemmaleigh added a commit that referenced this pull request Jun 13, 2017
If we drop the govuk- prefix from both, the elements import becomes
import “elements” to import all govuk-elements partials and import
“frontend-toolkit” to  import the govuk-frontend-toolkit dependencies.

The main govuk-elements stylesheet still imports the same partials,
these were split into two files in #489.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants