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

Create universal layout with no navigation variant #535

Merged
merged 8 commits into from Nov 14, 2017

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Nov 13, 2017

Raising this PR in case I need to handover the work at short notice.

Creates a universal layout for:

  • guides
  • answers
  • publications (statutory guidance only)
  • detailed guides

Tests are applied to these pages based on logic in #526
https://gist.github.com/fofr/cbf398e868ac2e3052be5a666635ee6a

When the "UniversalNoNav" variant is set:

  • no breadcrumbs display
  • no related links or taxonomy sidebar displays
  • no metadata or document footer components display
  • all content is inline in a left column, with an empty column to the right

Still to do:

  • Incorporate metadata component #534

To test on Heroku:

Part of:
https://trello.com/c/1caGTIJn/77-create-no-navigation-test-variant-based-on-first-universal-design
Follows on from #526

Screenshots

Type Screen
Mobile guidance government-frontend-pr-535 herokuapp com-guidance-schools-financial-efficiency-effective-procurement iphone 6
Desktop guidance government-frontend-pr-535 herokuapp com-guidance-schools-financial-efficiency-effective-procurement
Desktop guide government-frontend-pr-535 herokuapp com-apprenticeships-guide 1
Desktop publication government-frontend-pr-535 herokuapp com-government-publications-send-code-of-practice-0-to-25
@boffbowsh boffbowsh temporarily deployed to government-frontend-pr-535 Nov 13, 2017 Inactive
Introduce methods for setting up content item examples that are in or
out of content navigation test.
@fofr fofr force-pushed the content-navigation-universal-layout branch from c90db8f to 553f42d Nov 14, 2017
@fofr fofr temporarily deployed to government-frontend-pr-535 Nov 14, 2017 Inactive
@fofr fofr temporarily deployed to government-frontend-no-nav Nov 14, 2017 Inactive
@fofr fofr changed the title [WIP] Create universal layout with no navigation variant Create universal layout with no navigation variant Nov 14, 2017
Copy link
Contributor

@vanitabarrett vanitabarrett left a comment

Looks good 👍

Just 2 small comments - something I spotted with spacing and possibly something that could be included in a test.

refute_partial('govuk_component/document_footer')

if document_type.in? %w{detailed_guide statutory_guidance}
assert_template 'components/_published-dates'

This comment has been minimized.

@vanitabarrett

vanitabarrett Nov 14, 2017
Contributor

Do we need to check for the new metadata component too?

This comment has been minimized.

@fofr

fofr Nov 14, 2017
Author Contributor

Yep we should, I will add

<div class="grid-row">
<div class="column-two-thirds">
<div class="responsive-bottom-margin">
<%= render 'components/contents-list', contents: @content_item.contents %>

This comment has been minimized.

@vanitabarrett

vanitabarrett Nov 14, 2017
Contributor

In some guides, the responsive-bottom-margin div is rendering with nothing in it, making the gap between metadata and content look quite large. I think this is happening when there's no contents list, but the wrapping div still renders?

https://government-frontend-pr-535.herokuapp.com/guidance/introduction-of-statutory-participation-in-trialling-of-national-curriculum-tests-from-2016

screen shot 2017-11-14 at 13 23 04

@maxgds
Copy link
Contributor

@maxgds maxgds commented Nov 14, 2017

You say "all content is inline in a left column, with an empty column to the right" but this doesn't apply to headers across the board. Seems like urls with /guidance/ in them constrain the heading to the column, but others, like /adding-fathers-name-birth-certificate have full width headers. Do we need to adjust this? The headers also have different treatment - larger text, more impact.

fofr added 6 commits Nov 9, 2017
* Don’t render breadcrumbs
* Use consistent two column layout
* Ignore all navigation elements in no nav variant
* Constrain in two-thirds column for good line length on history
* Avoid running 10 tests for these document types, one will suffice
* When developing locally this number can be increased for confidence
* Increase chances of spotting regressions by ensuring that the
`setup_and_visit_random_content_item` test will load pages that
sometimes qualify for the AB test
* Add more examples showing how the component renders without publishers
* Include details about the component’s purpose and where used, and
that it references another component
* Fix bug with published dates having a margin that means a date only
metadata component looks odd
* Include component as part of universal layout
* Add margins to published dates component at foot of page
@fofr fofr force-pushed the content-navigation-universal-layout branch from 553f42d to df7cb79 Nov 14, 2017
@fofr fofr temporarily deployed to government-frontend-no-nav Nov 14, 2017 Inactive
@fofr fofr temporarily deployed to government-frontend-pr-535 Nov 14, 2017 Inactive
@fofr
Copy link
Contributor Author

@fofr fofr commented Nov 14, 2017

@vanitabarrett Fixed those two bugs and rebased, good spots.

@fofr
Copy link
Contributor Author

@fofr fofr commented Nov 14, 2017

@maxgds The detailed guides could potentially have translations, which would show in the top right, hence the constraint.

Spoke to @alextea about the different title sizes (based on average length of post) – he was happy to leave as-is for now. I think in the future we might want to base this setting on the length of the title.

The top margin has no effect on most pages – it’s increased by the
title that lives above it.

Except on the new universal navigation variant where the margin appears
too large because the margins aren’t collapsing.

The 30px margin is still needed on mobile
@fofr fofr merged commit 736b79f into master Nov 14, 2017
2 checks passed
2 checks passed
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/publishing-e2e-tests Publishing end-to-end tests succeeded on Jenkins
Details
@fofr fofr deleted the content-navigation-universal-layout branch Nov 14, 2017
@alextea
Copy link
Contributor

@alextea alextea commented Nov 16, 2017

I misunderstood @fofr's question when he asked about heading size (I thought he was referring to the font size). All headings should be constrained to 2/3 width.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.