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

Add handling of for_static parameter #4255

Merged
merged 1 commit into from
Nov 11, 2024
Merged

Conversation

KludgeKML
Copy link
Contributor

  • original behaviour was to always include wrapper class unless show_account_layout or custom_layout was present (custom layout is as of yet not used). Alter behaviour so that default is not to have the wrapper, and it's only included if for_static is true.

What

Why

layout_for_public does not behave like a standard layout - it's default is to include a wrapper element which is cut out by slimmer and replaced with the wrapper from the client app. If we try to use layout_for_public directly, this results in a double wrapper (or a single wrapper, but without the ability to add classes to it). Adding a parameter which maintains this behaviour (which can be added in Static: alphagov/static#3440 ), but defaulting to not including the wrapper element allows other apps to use the layout component directly.

https://trello.com/c/COQYfYgn/347-allow-layoutforpublic-to-work-like-a-normal-layout

Visual Changes

None expected

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4255 September 24, 2024 13:36 Inactive
@KludgeKML KludgeKML marked this pull request as ready for review November 6, 2024 16:17
Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

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

Probably need a CHANGELOG entry, but otherwise looks good 👍

@KludgeKML KludgeKML force-pushed the add-for-static-parameter branch 4 times, most recently from 1323959 to f670c0f Compare November 7, 2024 14:22
- original behaviour was to always include
  wrapper class unless show_account_layout or
  custom_layout was present (custom layout is as
  of yet not used). Alter behaviour so that default is
  not to have the wrapper, and it's only included if
  for_static is true.
- Update examples
- Note that this is likely to cause a number of
  visual regressions in the test suite as only some
  of the examples have the for_static: true flag set.
- The visual regression tester currently relies on
  the wrapper being present to take its screenshot,
  so we add the for_static: true flag to the layout
  in the dummy app.
@KludgeKML
Copy link
Contributor Author

Have approved the Percy visual regression - it's one added example (with the for_static parameter), then the other examples getting shrunk slightly because they don't have the for_static: true flag set, so they are missing the wrappers and main elements (the point of this PR).

@KludgeKML KludgeKML merged commit a3f5aaa into main Nov 11, 2024
12 checks passed
@KludgeKML KludgeKML deleted the add-for-static-parameter branch November 11, 2024 10:48
@KludgeKML KludgeKML mentioned this pull request Nov 11, 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.

3 participants