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 display override for hiding content when printing #1723

Merged
merged 1 commit into from
Feb 27, 2020

Conversation

fofr
Copy link
Contributor

@fofr fofr commented Feb 4, 2020

When printing users often need to print the content without the surrounding site furniture (eg menus, breadcrumbs, back links and footers).

Introduce a helper so that a class can be used to hide any given element from the print view:

govuk-!-print-display-none

This is explicit and preferable to listing all the selectors for things that shouldn't be printed. These lists grow unwieldy over time. (An example of one of these lists)

(We've been using this pattern on Apply for teacher training and Becoming a teacher: Design History)

@NickColley NickColley added awaiting triage Needs triaging by team feature request User requests a new feature labels Feb 4, 2020
@NickColley
Copy link
Contributor

@fofr what do you think about components such as the footer having sensible print styles: #573 ? Do you think you'd want full control over that with a helper instead of them being baked in?

@fofr
Copy link
Contributor Author

fofr commented Feb 7, 2020

@NickColley I think we need both.

Components should have good print styles out of the box. But sometimes it's not clear if a component should be printed or not – for instance a breadcrumb may sometimes be useful to print as it shows the hierarchy or context of some content, whereas at other times it's simply a navigational aid and can be hidden.

In our case we are printing an application and all its completed fields. Which means hiding a lot from the print view so that it's clean and appears similar to a completed paper form.

@timpaul timpaul removed the awaiting triage Needs triaging by team label Feb 10, 2020
@NickColley
Copy link
Contributor

We're happy to accept this, would you mind adding a feature CHANGELOG entry that introduces the new class? You can look at other feature CHANGELOG entries for examples.

I can do this on your behalf if you'd prefer that.

@fofr
Copy link
Contributor Author

fofr commented Feb 10, 2020

I've added a changelog entry. Happy for you to amend if it's not quite right.

@@ -19,4 +19,10 @@
.govuk-\!-display-none {
display: none !important;
}

@include govuk-media-query($media-type: print) {
.govuk-\!-print-display-none {
Copy link
Contributor

@NickColley NickColley Feb 10, 2020

Choose a reason for hiding this comment

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

Sorry for this after approving things.

Can we be careful to consider this naming vs all the other overrides?

Perhaps this should be .govuk-\!-display-none-print as the format seems to be .govuk-\!-<PROPERTY>-<VALUE/SCALE>-<MEDIA>

Copy link
Contributor

Choose a reason for hiding this comment

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

I've pinged the team on this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We put print first so that it was more easily differentiated from govuk-!-display-none.

print-display-none matches the order in the stylesheet:

@media print {
  .blah {
    display: none
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@36degrees 36degrees Feb 12, 2020

Choose a reason for hiding this comment

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

As far as I can see, we don't have any other overrides that are conditional on media queries. Probably the closest existing class would be e.g. govuk-grid-column-one-third-from-desktop, where the 'media query' part is last?

If we did go with govuk-!-display-none-print that means it would also match if you searched your code base for govuk-\!-display-none, but that's also true for govuk-grid-column-one-third-from-desktop and govuk-grid-column-one-third and I'm not aware of that having been an issue for anyone.

@fofr
Copy link
Contributor Author

fofr commented Feb 19, 2020

What's happening with this PR now?

@NickColley
Copy link
Contributor

@fofr sorry I think I got us confused and left this in an unresolved place.

I'll work with the team to get consensus and reply to you, thanks for your patience.

@NickColley
Copy link
Contributor

@fofr sorry for the delay, we decided that in order to keep consistency with the existing govuk-grid-column-one-third-from-desktop class that we want to name this new override govuk-!-display-none-print. If you want we can do this on your behalf, let us know :)

When printing users often need to print the content without the surrounding site furniture (eg menus, breadcrumbs, back links and footers).

Introduce a helper so that a class can be used to hide any given element from the print view.

This is explicit and preferable to listing all the selectors for things that shouldn't be printed. These lists grow unwieldy over time.
@fofr
Copy link
Contributor Author

fofr commented Feb 27, 2020

@NickColley PR updated.

Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Thanks Paul, this is a nice feature.

@NickColley NickColley merged commit 2b3f2a3 into alphagov:master Feb 27, 2020
@hannalaakso hannalaakso modified the milestones: Next, v3.6.0 Mar 3, 2020
@36degrees 36degrees mentioned this pull request Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request User requests a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants