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.

Sign up for GitHub

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

Jump to bottom

Fix print issues on layout-super-navigation-header #4165

Merged
merged 2 commits into from
Aug 23, 2024
Merged

Fix print issues on layout-super-navigation-header #4165

merged 2 commits into from
Aug 23, 2024

Conversation

matthillco
Copy link
Contributor

@matthillco matthillco commented Aug 20, 2024 •
edited
Loading

What

The print styles on the layout-super-navigation-header are incorrect and need to be updated:

  • Background should always print transparent
  • Foreground should always print black
  • No top border
  • Bottom border should match new changes in cross service header
  • Left/right margins are not required for print due to background removal

Trello

Why

  • The top bar has an extra top border and redundant left/right spacing when printed, it also doesn’t match the changes made to the cross service header (coming in Update and/or add component print styles #4073)
  • The background color bug was recently introduced in the previous print styles work and needs fixing: it prevents the inverted background from printing correctly.

Visual Changes

Before

image

After

image

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4165 August 20, 2024 16:12 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4165 August 20, 2024 16:15 Inactive
nnagewad
nnagewad reviewed Aug 21, 2024
View reviewed changes
Copy link

@nnagewad nnagewad left a comment

Choose a reason for hiding this comment

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

Just wondering why the homepage doesn't have text?

nnagewad
nnagewad reviewed Aug 21, 2024
View reviewed changes
Copy link

@nnagewad nnagewad left a comment

Choose a reason for hiding this comment

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

Just wondering is there a screenshot available of a GOV.UK page with the updated print styles?

Be good to see it within the context an end-user experiences it.

@matthillco
Copy link
Contributor Author

matthillco commented Aug 21, 2024 •
edited
Loading

Just wondering why the homepage doesn't have text?

@nnagewad I did originally write some new print styles to ensure that all the headers looked the same with the same "GOV.UK" text next to the crown logo. However during review @andysellick suggested it wasn't necessary to do that since the homepage is an edge case where the "GOV.UK" text is part of the title content, and the crown is left as a single ident.

The problem now is that by having the border underneath the crown/logo, it looks good for most pages but might look odd if the homepage is printed.

@matthillco
Copy link
Contributor Author

matthillco commented Aug 21, 2024 •
edited
Loading

Just wondering is there a screenshot available of a GOV.UK page with the updated print styles?

Be good to see it within the context an end-user experiences it.

@nnagewad I'll sort out a preview app of frontend for you to look at this, I'll let you know when it's available.

@matthillco matthillco force-pushed the print-styles-layout-supernav-header branch from c0d8cf4 to dd02ba2 Compare August 21, 2024 09:46
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4165 August 21, 2024 09:47 Inactive
andysellick
andysellick previously approved these changes Aug 21, 2024
View reviewed changes
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.

Looks good to me 👍

Have checked in frontend as well, all looks okay.

@matthillco matthillco mentioned this pull request Aug 21, 2024
Closed
@matthillco
Copy link
Contributor Author

@nnagewad Here's before/after screenshots of the revised top header bar. (Note there's some other print changes on these pages, but we're not looking at those in this PR.)

Standard Page

image

Homepage

image

@nnagewad
Copy link

nnagewad commented Aug 21, 2024 •
edited by matthillco
Loading

@nnagewad Here's before/after screenshots of the revised top header bar. (Note there's some other print changes on these pages, but we're not looking at those in this PR.)

This looks really good. Just wondering, is it worth having the horizontal rule on the homepage?

From your experience, how difficult will it be to remove it?

@matthillco matthillco force-pushed the print-styles-layout-supernav-header branch from dd02ba2 to 49bad0a Compare August 22, 2024 13:57
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4165 August 22, 2024 13:57 Inactive
@matthillco matthillco force-pushed the print-styles-layout-supernav-header branch from 49bad0a to cadfb48 Compare August 22, 2024 13:59
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4165 August 22, 2024 13:59 Inactive
@matthillco matthillco force-pushed the print-styles-layout-supernav-header branch from cadfb48 to 061a96f Compare August 22, 2024 14:19
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4165 August 22, 2024 14:20 Inactive
@matthillco
Copy link
Contributor Author

matthillco commented Aug 22, 2024 •
edited
Loading

@nnagewad This is updated now so that variants for the homepage will not show the border, and the logo is indented to align with the intro text below:
image

@matthillco matthillco dismissed andysellick’s stale review August 22, 2024 14:41

Code has been updated to support a design change.

@matthillco matthillco force-pushed the print-styles-layout-supernav-header branch from 061a96f to 29e2fe2 Compare August 22, 2024 14:57
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4165 August 22, 2024 14:57 Inactive
@matthillco matthillco force-pushed the print-styles-layout-supernav-header branch from 29e2fe2 to 9cc48dd Compare August 22, 2024 15:01
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4165 August 22, 2024 15:02 Inactive
@matthillco matthillco requested a review from AshGDS August 23, 2024 09:20
@matthillco
Fix print issues on layout-super-navigation-header
f5b1365 
- Background should always print transparent
- Foreground should always print black
- Remove top border
- Add bottom border to match new changes in cross service header
- Remove left/right margins not required for print

Note the use of the :has selector to remove the border and add a left indent
only if the component contains the hidden 'GOV.UK' text. Variants that have
hidden logo text are only used on the homepage, and need an adjusted design
for print so that the crown logo lines up with the intro text below.
@matthillco
Update CHANGELOG.md
ac51e19
@matthillco matthillco force-pushed the print-styles-layout-supernav-header branch from 9cc48dd to ac51e19 Compare August 23, 2024 10:00
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4165 August 23, 2024 10:00 Inactive
@matthillco matthillco merged commit 174cd7e into main Aug 23, 2024
12 checks passed
@matthillco matthillco deleted the print-styles-layout-supernav-header branch August 23, 2024 10:05
@AshGDS AshGDS mentioned this pull request Aug 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andysellick andysellick andysellick left review comments

@nnagewad nnagewad nnagewad approved these changes

@AshGDS AshGDS AshGDS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@matthillco @nnagewad @andysellick @AshGDS @govuk-ci