-
Notifications
You must be signed in to change notification settings - Fork 17
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 "print entire guide" functionality #2315
Conversation
6556290
to
5a052b9
Compare
5a052b9
to
2530046
Compare
2530046
to
30bffa9
Compare
30bffa9
to
3078c70
Compare
3078c70
to
ae9ac66
Compare
ae9ac66
to
f08834f
Compare
f08834f
to
880b7ad
Compare
880b7ad
to
6b0ac00
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small thing for you to consider - happy to approve it if you and Nik decide it's ok as is.
Good point on the page structure, I'll raise another ticket for that. Please update the links on it, though, currently they point to the same example for both schema types.
title: @content_item.title, | ||
} %> | ||
|
||
<%= render 'govuk_publishing_components/components/lead_paragraph', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might benefit from adding govuk-!-display-none-print which means while it renders on screen it wouldn't show up in the printed version - one to check with Nik though. I think you'll have to use a wrapper to apply it as I don't think the lead paragraph can, or should, take extra classes.
I was in two minds about using the lead paragraph component as this doesn't seem to qualify as a paragraph, it's so short, but it does seem to match the use case when I looked at the docs so I think it should stay even if that does mean we need the extra clutter of a wrapper if we hide the text on the printout.
Whatever you do here obviously applies for the other case where it appears. While the same is true for the actual print button, that's handled by the component side and already applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke to @jon-kirwan and really like that idea @maxgds. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @maxgds - have updated both page / schema types as above. I can't see a way to pass in an extra class to the lead paragraph component either so instead have wrapped it with a div
and added the govuk-!-display-none-print
class.
I've updated in the same commit since it's quite a small change.
Also updated the link examples in the description.
6b0ac00
to
3d12664
Compare
What
https://trello.com/c/OlsybM1y/914-update-print-entire-guide-functionality
/print
)/print
)h1
, Printable version using the lead paragraph component/print
)Why
Visual changes
Anything else
"schema_name": "guide"
. I also found that the print this page functionality can be found on page types of"schema_name": "travel_advice"
e.g. https://www.gov.uk/foreign-travel-advice/afghanistan"schema_name": "travel_advice"
) has twoh1
s (I'm unable to fix this at the moment since I think content changes would also be needed). You can see the same problem here: https://www.gov.uk/universal-credit ("schema_name": "guide"
). See:government-frontend/app/views/content_items/guide.html+print.erb
Lines 33 to 35 in 6b0ac00
and
government-frontend/app/views/content_items/travel_advice.html+print.erb
Lines 32 to 34 in 6b0ac00