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 static to use component gem #43

Merged
merged 5 commits into from Aug 31, 2017
Merged

Allow static to use component gem #43

merged 5 commits into from Aug 31, 2017

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Aug 31, 2017

Static components currently live in a different directory to app components. (“govuk_component” vs “components”) While this discrepancy exists the gem needs to know whether it’s documenting static components or app components.

Some oddities were also found concerning the way dependencies are required when Slimmer is or isn't there:

  • As static doesn't use slimmer, we need to explicitly require it: 9c812fa – tests continued to pass locally but needed to be updated with ba29030 to pass on CI (this was because of different Slimmer versions locally and on CI)
  • Static didn't always load dependencies from govuk_frontend_toolkit when they were referenced from another file – putting them into a single file avoided the problem but the cause wasn't identified (ab888f1)

(Gem changes tested against government-frontend and static)

Goes with alphagov/static#1125

fofr added 5 commits Aug 29, 2017
Do not depend on parent application already having started Slimmer (eg
Static will not)
Static components currently live in a different directory to app
components. (“govuk_component” vs “components”)

While this discrepancy exists the gem needs to know whether it’s
documenting static components or app components.
Static behaves strangely when autoloading govuk_frontend_toolkit and
importing the component_guide.

Somehow the component guide sass file was loaded outside of the scope
of the toolkit, so did not have access to things like $gutter and
core-19.

By moving into a single file we avoid this problem. Although the
initial cause is still unclear.
Allow components to be generated in static, taking account of subtle
inconsistencies that currently exist. In static components:

* use the govuk_component directory
* don’t begin with an underscore
* include `.raw` in their filename
* use underscores in doc and partial filenames
* use hyphens in styles
* store styles in govuk-component rather than govuk_component
* use the `pub-c-` prefix
Avoid making network requests to slimmer when running tests.

After slimmer was explicitly required in engine, the tests
attempted to use slimmer. This only happened on CI, not
locally.
@h-lame
h-lame approved these changes Aug 31, 2017
Copy link
Contributor

@h-lame h-lame left a comment

Other than the comment I've left on the .scss file changes the rest of this looks good.

@@ -4,4 +4,176 @@
@import "typography";
@import "colours";

@import "component_guide";
$prism-background: #f5f2f0;

This comment has been minimized.

@h-lame

h-lame Aug 31, 2017
Contributor

It's not 100% clear to my why this change is required, but I can shed a little more light on the situation which may help others who know more about sass and sprockets. If we add a variable to application.scss and try to reference it in component_guide.scss it doesn't work, so the failure isn't that we're not loading in the frontend toolkit variables properly; it's that we're not sharing any variables at all.

If we move component_guide.scss into a subfolder (e.g. /app/assets/stylesheets/govuk_publishing_components/whyyyy and change the import statement accordingly (e.g. @import "whyyyy/component_guide";) then the stylesheet does render and can reference the variables imported from govuk_frontend_toolkit or added directly here.

This problem only surfaced when we used this gem in static, and not when we used it in government-frontend, so I suspect it's down to differences in the versions of gems used in the asset-pipeline: sass, sass-rails, sprockets, sprockets-rails, and rails itself.

In either case we have to make a large amount of git diff noise to move the file around so it's not clear which is better.

@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 31, 2017

Thanks @h-lame. Merging this to unblock alphagov/static#1125

@fofr fofr merged commit 0ffeab9 into master Aug 31, 2017
1 check passed
1 check passed
continuous-integration/jenkins/branch This commit looks good
Details
@fofr fofr deleted the static-components branch Aug 31, 2017
fofr added a commit that referenced this pull request Aug 31, 2017
* Allow gem to be used with static (PR #43)
@fofr fofr mentioned this pull request Aug 31, 2017
fofr added a commit to alphagov/static that referenced this pull request Aug 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants