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

Update/add component print styles #1934

Merged
merged 1 commit into from
Aug 5, 2024
Merged

Update/add component print styles #1934

merged 1 commit into from
Aug 5, 2024

Conversation

matthillco
Copy link
Contributor

@matthillco matthillco commented Aug 1, 2024

What

Hide the promotion element when printing "transaction done" pages. Trello card

Example page: https://www.gov.uk/done/transaction-finished

Why

This change hides the grey promotion element when printed as it's redundant in a print context: the important part of that promotion is the link, which is pointless on a printout.

NOTE: There's also an argument for hiding the Related Content as that's redundant when printed too, however this has not been changed as it might have cascading consequences for other pages.

SCREEN (BEFORE)

image

PRINT (AFTER)

image

@govuk-ci govuk-ci temporarily deployed to feedback-pr-1934 August 1, 2024 15:42 Inactive
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.

@matthillco can you explain what this change is? The PR description appears to be the generic work description copied from the Trello card, but this specific change is hiding a heading when printing. I'd like to know which page this is on and why you're making this change.

@matthillco
Copy link
Contributor Author

matthillco commented Aug 2, 2024

@andysellick

The PR description appears to be the generic work description copied from the Trello card

I've kept the PR description the same in all these apps for consistency. The idea was to explain the PR according to the Trello card, then have each commit be specific about each component change. It worked well in other apps with multiple commits, but here it's just one commit.

I'd like to know which page this is on and why you're making this change.

The page is referenced in the tracking spreadsheet which is linked on the Trello card, eg: https://www.gov.uk/done/transaction-finished

This change is simply hiding the grey promotion element when printed as it's basically redundant: the important part of that promotion is the link, which is pointless on a printout. We've used the same logic for hiding other elements in other apps too.

There's also an argument for hiding the Related Content as that's redundant when printed too, however I didn't want to change that here as it might have cascading consequences for other pages.

Of course, this is a fairly useless page for anyone to print anyway, but at least it's covered now.

SCREEN

image

PRINT

image

@andysellick
Copy link
Contributor

@matthillco great, thank you for that clear description. Please make that the PR description, and update the PR title accordingly.

@andysellick
Copy link
Contributor

@matthillco please remember that we use github as a history of our changes. Linking to relevant trello cards is useful, but only for background information, as these boards may not be accessible by others or exist forever. A PR description and commit messages should fully encapsulate everything someone needs to know in order to review the change being made.

@matthillco
Copy link
Contributor Author

@matthillco great, thank you for that clear description. Please make that the PR description, and update the PR title accordingly.

@andysellick I've updated the title and description as requested.

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.

Thanks @matthillco

@matthillco matthillco merged commit 6eeffdf into main Aug 5, 2024
12 checks passed
@matthillco matthillco deleted the update-print-styles branch August 5, 2024 14:46
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