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

Rework the way we include CSS & JS #571

Merged
merged 14 commits into from
Oct 11, 2018
Merged

Rework the way we include CSS & JS #571

merged 14 commits into from
Oct 11, 2018

Conversation

tijmenb
Copy link
Contributor

@tijmenb tijmenb commented Oct 9, 2018

This is another attempt at unfuzzing the way we include CSS & Javascript. This PR reduces it down to the following:

  • All applications should include the CSS themselves. Public frontend applications already do this, but the admin apps (Signon, Content Publisher and Content Data Admin) should be updated to do this too. The admin layout component doesn't include CSS automatically anymore, instead it uses the application's stylesheet (application.css by convention). This means public and admin apps use the exact same mechanism to load the CSS.
  • For Javascript, the same applies. Except that Slimmer provides some core dependencies that admin apps have to include for themselves.

See this pull request on how to convert the application.

@benthorner benthorner temporarily deployed to govuk-publishing-compon-pr-571 October 9, 2018 15:08 Inactive
@benthorner benthorner temporarily deployed to govuk-publishing-compon-pr-571 October 9, 2018 15:52 Inactive
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Looks good thanks for carrying this through.

Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

💯 This looks great!
I agree with @kevindew's suggestions, other than that can't wait to get it in.

@benthorner benthorner temporarily deployed to govuk-publishing-compon-pr-571 October 11, 2018 10:21 Inactive
@benthorner benthorner temporarily deployed to govuk-publishing-compon-pr-571 October 11, 2018 10:26 Inactive
We're using the tag component, but we've previously relied on the
`govuk-frontend/all` import to import the CSS.
This is no longer needed, because all components are renderable in all
contexts.
This imports the CSS that we need for the layout into the
layout-for-admin component. This means that the `govuk-grid-*`,
`govuk-link` and `govuk-heading` classes are available inside this
component, but not outside. This means we can stop including all of
govuk-frontend using `@import "govuk-frontend/all"`.
This removes the separate "admin_styles.css" file for the components.
From now on the applications will have to have an `application.css`
file with the correct component includes.
This component now has a class.
This removes the separate "admin_styles.js" file for the components.
From now on the applications will have to have an `application.js` file
with the correct component includes. Updated the installation
instructions as well.
These files don't need to be available separately.
This was relying previously on the `govuk-frontend/all` import.
This is needed by content publisher to hide certain content.
The component guide already includes these.
@benthorner benthorner temporarily deployed to govuk-publishing-compon-pr-571 October 11, 2018 12:49 Inactive
This file is already required by
`govuk_publishing_components/dependencies`.
Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

👏

@tijmenb tijmenb merged commit c0f608d into master Oct 11, 2018
@tijmenb tijmenb deleted the rework-includes branch October 11, 2018 13:45
alex-ju added a commit that referenced this pull request Oct 12, 2018
* Add option to send the `user_organisation` in admin analytics component (PR #577)
* Add a bottom margin to the error-alert component (PR #578)
* Update the way we include Javascript and Stylesheets in the admin layout
  component. Make sure to follow the [installation instructions](docs/install-and-use.md) (PR #571) if your using the admin layout component.
* Fix background colour for focused buttons (PR #579)
@alex-ju alex-ju mentioned this pull request Oct 12, 2018
tijmenb pushed a commit that referenced this pull request Oct 15, 2018
* Add option to send the `user_organisation` in admin analytics component (PR #577)
* Add a bottom margin to the error-alert component (PR #578)
* Update the way we include Javascript and Stylesheets in the admin layout
  component. Make sure to follow the [installation instructions](docs/install-and-use.md) (PR #571) if your using the admin layout component.
* Fix background colour for focused buttons (PR #579)
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.

None yet

4 participants