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

Configure asset helper to load individual stylesheets #3029

Merged

Conversation

MartinJJones
Copy link
Contributor

@MartinJJones MartinJJones commented Mar 28, 2023

What

Changes to load component and view style sheets only required on the page being viewed.

Trello card: https://trello.com/c/ZiXZCjnZ/1871-enable-individual-loading-of-stylesheets-in-finder-frontend

Why

This reduces the amount of CSS required for each page and increases the ability of a browser to cache the stylesheets - this should mean a faster load time for both first time and repeat visitors.

See RFC #149 for more details.

Review URL(s)

Anything else

  • Print styles have moved to the finder-frontend stylesheet and the print.css file was removed, this approach is consistent with other frontend rendering apps
  • Removed email-link component as it was not in use
  • Removed date-filter stylesheet as it is not required. The only styles present were to set a bottom margin, however these were always overridden by the .govuk-!-margin-bottom-0 class in the view template
  • Looked at removing the Eric Meyer browser reset, originally added in Add organisation link to finder #33, but this would require quite a bit of refactoring in other app components, I'll create a separate issue for this.

Issues fixed

option-select component not rendered correctly on subsequent page visits

When testing the finder on both Integration and Heroku, the option-select component was not rendering correctly after the first page visit.

This was down to the caching of the partial. When the partial is cached, the helper methods to add the required stylesheets are not called. By calling the methods before caching is enabled, we ensure that the stylesheets are always requested.

Fix option-select component rendering issue

Correct margin-bottom not applied to govuk-checkboxes__label

govuk-checkboxes__label sets margin-bottom: 5px, one some pages this was overridden by the margin set in govuk-label. This was fixed by using the existing override approach in the application and nesting .govuk-checkboxes__label within .finder-frontend-content to give it a higher specificity value - Ensure checkbox labels always have 0 bottom-margin

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

Follow these steps if you are doing a Rails upgrade.

@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 28, 2023 10:08 Inactive
@MartinJJones MartinJJones added the do-not-merge Indicates that a PR should not be merged into master / release branches label Mar 28, 2023
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 28, 2023 10:53 Inactive
@MartinJJones MartinJJones force-pushed the configure-asset-helper-to-load-individual-stylesheets branch from 21d7c84 to 8cd8bdb Compare March 28, 2023 11:16
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 28, 2023 11:16 Inactive
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 28, 2023 11:26 Inactive
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 28, 2023 11:55 Inactive
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 28, 2023 17:46 Inactive
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 29, 2023 12:09 Inactive
@MartinJJones MartinJJones force-pushed the configure-asset-helper-to-load-individual-stylesheets branch from f196bec to bdc663c Compare March 29, 2023 12:18
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 29, 2023 12:18 Inactive
@MartinJJones MartinJJones force-pushed the configure-asset-helper-to-load-individual-stylesheets branch from bdc663c to 26a6d19 Compare March 29, 2023 13:12
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 29, 2023 13:13 Inactive
@MartinJJones MartinJJones force-pushed the configure-asset-helper-to-load-individual-stylesheets branch from 26a6d19 to 793fb2d Compare March 29, 2023 13:48
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 29, 2023 13:48 Inactive
@MartinJJones MartinJJones force-pushed the configure-asset-helper-to-load-individual-stylesheets branch from 793fb2d to efac733 Compare March 30, 2023 08:36
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 30, 2023 08:36 Inactive
@MartinJJones MartinJJones changed the title [WIP] Configure asset helper to load individual stylesheets Configure asset helper to load individual stylesheets Mar 30, 2023
@MartinJJones MartinJJones marked this pull request as ready for review March 30, 2023 08:41
@MartinJJones MartinJJones force-pushed the configure-asset-helper-to-load-individual-stylesheets branch from efac733 to eb2ab0d Compare March 30, 2023 09:50
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 30, 2023 09:50 Inactive
@MartinJJones MartinJJones force-pushed the configure-asset-helper-to-load-individual-stylesheets branch from eb2ab0d to 9abde83 Compare March 30, 2023 09:51
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 30, 2023 09:51 Inactive
@MartinJJones MartinJJones force-pushed the configure-asset-helper-to-load-individual-stylesheets branch from 9abde83 to 11df615 Compare March 30, 2023 10:02
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 March 30, 2023 10:03 Inactive
Copy link
Contributor

@KludgeKML KludgeKML 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 to me. Nicely broken down commits, as well!

Copy link
Contributor

@jon-kirwan jon-kirwan left a comment

Choose a reason for hiding this comment

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

Nice work - all looks good to me!

Print styles have moved to the `finder-frontend` stylesheet below the related class name.

Print styles for `.document`, `.document-metadata` and `.feed` were removed as they did not target any elements.
- Hoist body content in application layout
- Call AssetHelper helper `render_component_stylesheets`
- Remove GOV.UK Component stylesheets from application stylesheet
The only styles present for `date-filter` were to set values for margin-bottom, however these were always overridden by the `.govuk-!-margin-bottom-0` utility class in the view template.

Remove `email-link` component as it was not used in the application.
- Include the select component stylesheet manually, since the `gem-c-select` is currently hardcoded in the view template, the stylesheet is not automatically included
- Nested the style rules for `sort-options__label` and `sort-options__select` within `sort-options`. This was done to
 fix styling issues by increasing the CSS specificity to override values set in `.govuk-label`
Ensure the override for `margin-bottom` is applied. This is done by nesting the rule for `.gem-c-intervention` within `.finder-frontend-content` to increase CSS specificity.
Nest gem-c overrides in `.finder-frontend-content`.

Although I could not see any issues when testing, it makes sense to also move the other overrides now to help avoid any issues when making changes in the future.
When the partial is cached, the helper methods called to add the stylesheets are not called. By calling the methods before caching is enabled, we ensure that the stylesheets are always requested.
@jon-kirwan jon-kirwan force-pushed the configure-asset-helper-to-load-individual-stylesheets branch from 11df615 to 5da81f5 Compare April 13, 2023 09:24
@govuk-ci govuk-ci temporarily deployed to finder-frontend-pr-3029 April 13, 2023 09:25 Inactive
@jon-kirwan jon-kirwan merged commit 53cf049 into main Apr 13, 2023
@jon-kirwan jon-kirwan deleted the configure-asset-helper-to-load-individual-stylesheets branch April 13, 2023 09:30
@MartinJJones MartinJJones removed the do-not-merge Indicates that a PR should not be merged into master / release branches label Feb 20, 2024
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.

4 participants