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

Group examples and their assets in directories #274

Merged
merged 1 commit into from
May 24, 2018
Merged

Conversation

alex-ju
Copy link
Contributor

@alex-ju alex-ju commented May 17, 2018

This PR restructures src so that:

  • avoids assets duplication when deployed
  • minimises the size of the assets being downloaded (which also helps workbox-service-worker by @Nooshu to have a minimum size)
  • each example sits with its own assets in a folder, so it's clear by which is being used
  • common/shared assets are being used at the component/pattern level

Below is an example of the output for the Addresses pattern

Output

Before

.
├── address-wrapper.css
├── index.html
├── multiple
│   ├── address-wrapper.css
│   ├── index.html
│   └── passports-address-lookup-1_.gif
├── passports-address-lookup-1_.gif
└── textarea
    ├── address-wrapper.css
    ├── index.html
    └── passports-address-lookup-1_.gif

After

.
├── index.html
├── multiple
│   ├── address-wrapper.css
│   └── index.html
├── passports-address-lookup-1_.gif
└── textarea
    └── index.html

@govuk-design-system-ci
Copy link
Collaborator

govuk-design-system-ci commented May 17, 2018

You can preview this change here:

Built with commit f3d1674

https://deploy-preview-274--govuk-design-system-preview.netlify.com

@Nooshu
Copy link
Contributor

Nooshu commented May 18, 2018

Great change @alex-ju, this really helps with the repetitive code a across the codebase. I investigated and solved it in a slightly different way.

I changed the metalsmith.js file to:

.use(sass({
    includePaths: ['node_modules'], // an array of paths that sass can look in when attempt to resolve @import declarations
    outputDir: 'stylesheets/'
}))

and views/layouts/layout-example.njk to:

<link href="/stylesheets/{{ stylesheet }}" rel="stylesheet" media="all" />

Essentially all the CSS is being generated into the root stylesheets directory then it is referenced from there. But this may be a massive oversimplification and may have unknown consequences when it comes to the deployment. So I'm happy to take your lead on these changes.

@alex-ju
Copy link
Contributor Author

alex-ju commented May 18, 2018

@Nooshu that's a slick approach; the only downside is that it's only dealing with the CSS. We may be able to replicate the behaviour (grabbing files from different sources and place it into one folder when published) for JavaScript, but unfortunately it won't solve the generic assets problem (i.e. images, archives, etc).

Based on your solution (and an alternative for grouping the assets in its own directory for each example) we can group all the assets across components/patterns/styles in a single assets folder at the root level.

I'm waiting for others opinion on this one and if the one above seems like a better approach I'll raise a separate PR with it.

@alex-ju alex-ju changed the title RFC: Group examples in their assets in their own directories RFC: Group examples and their assets in directories May 23, 2018
@alex-ju alex-ju force-pushed the group-example-assets branch 3 times, most recently from c8c5e6f to 126da9b Compare May 23, 2018 15:06
@alex-ju
Copy link
Contributor Author

alex-ju commented May 23, 2018

Would be good to have some feedback on this, otherwise I'll have to keep rebasing it for each example that gets merged.

@kr8n3r
Copy link
Contributor

kr8n3r commented May 23, 2018

I like this approach and hopefully update imports in these scss files won't bloat the filesize

@alex-ju alex-ju changed the title RFC: Group examples and their assets in directories Group examples and their assets in directories May 24, 2018
@alex-ju alex-ju merged commit 7c3d24b into master May 24, 2018
@alex-ju alex-ju deleted the group-example-assets branch May 24, 2018 14:07
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.

5 participants