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 examples #547

Merged
merged 7 commits into from
Jul 18, 2018
Merged

Update examples #547

merged 7 commits into from
Jul 18, 2018

Conversation

dashouse
Copy link

@dashouse dashouse commented Jul 11, 2018

This PR updates example pages in the kit to use GOV.UK Frontend spacing etc

Start Page

  • Updates the Start page to use the same code as the Design System (apart from <p> and <a> tags which remain without classes)
  • On top of that (separate commit) it then styles the Start page to be updated to the current GOV.UK start page (I'll be doing a PR for this on the Design System too) Not updating as related links design on GOV.UK isn't locked down.

Task list

  • Updates code to use spacing from GOV.UK Frontend and adds extra behaviour for mid and mobile breakpoints
  • Updates page layout to add class to main

screen shot 2018-07-11 at 11 06 00

Check answers

  • Updates <h1> to use govuk-heading-xl class
  • Removes native margin on the <dl> and sets spacing with GOV.UK Frontend

screen shot 2018-07-11 at 11 06 32

Confirmation page

  • Updates page layout to add class to main
  • Changes <h2> to <h1> in Panel (Next release GOV.UK Frontend)

screen shot 2018-07-11 at 11 06 42

Question and Content pages

  • Updates <h1> to use govuk-heading-xl class

screen shot 2018-07-11 at 11 05 48

Copy link

@kr8n3r kr8n3r 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. I'll let Joe have a look as well

@dashouse
Copy link
Author

I have removed the update Start Pages to new GOV.UK style commit after speaking to Mia.

The related links style is not completely locked down between GOV.UK apps and they are working on it more this quarter.

@joelanman joelanman self-requested a review July 12, 2018 09:49
@@ -1,15 +1,16 @@
{% extends "layout.html" %}

{% set mainClasses = "govuk-main-wrapper--l" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

what's this for? I can't see it being used

Copy link
Author

Choose a reason for hiding this comment

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

The pages without a back link, phase banner or breadcrumbs have a modifier to increase the padding-top (https://design-system.service.gov.uk/styles/layout/#exploded-view-of-page-wrappers-without-back-link-breadcrumbs-or-phase-banner)

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, can we add a comment to explain?

margin: 0;
padding-bottom: ($govuk-gutter / 6);
@include govuk-font($size: 24);
@include govuk-typography-weight-bold;
Copy link
Contributor

Choose a reason for hiding this comment

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

as discussed on slack this seems more complicated than the original approach:

@include govuk-font(24, $weight: bold);

@@ -1,5 +1,7 @@
{% extends "layout.html" %}

{% set mainClasses = "govuk-main-wrapper--l" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a comment to explain what this is doing?

Copy link
Contributor

@joelanman joelanman left a comment

Choose a reason for hiding this comment

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

Added a few comments, can we squash the two very similar heading commits?

@include govuk-font(19);

margin-top: 0;
padding-top: govuk-spacing(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm correct, we generally try to avoid top margin/padding - do we need this?

@joelanman
Copy link
Contributor

joelanman commented Jul 17, 2018

All looks good, just wondering if we want to change to heading-xl (48px) as the default for question pages? I think on One Thing Per Page it can overwhelm/be a bit unbalanced with the form elements. Previously Question pages had heading-l (36px).

Examples I've looked at online:

48px
Send money to prisoners

36px
Renew passports
MOT reminders

27px
Register to vote

@dashouse
Copy link
Author

dashouse commented Jul 17, 2018

@joelanman When making this decision I think we found as many services using 48px as page headings as not. One thing that came out of some research early on was there was a lot of confusion around inconsistent heading sizes in the Prototype Kit, GOV.UK publishing and services.

For example going from a Start page to a service...why does the heading change size?, which heading should I use?, what are the rules about when to use which heading size?

It seemed a lot of the debate came down to personal preference so when considering the spacing and the defaults (mapping classes to h-level elements in markdown / prose-scope) it was decided that 48px govuk-heading-xl should be the default as it was the most common in the user journey from GOV.UK > Service and is more inline with that mental mapping to an <h1>.

This was considered by re-implementing services, some examples (inc Register to vote) can be seen here https://designnotes.blog.gov.uk/2018/02/19/developing-new-typography-and-spacing-for-gov-uk-frontend/#attachment_2367

I should emphasise, we thought this should be the default...not a hard rule, if you need to reduce it for a particular question, service, admin interface then that's up to the user.

@joelanman
Copy link
Contributor

So I think given we've already made this change in the Design System, we should merge this to be consistent in the Prototype Kit. However I think we should revisit this decision - for long questions especially it can be unbalanced with the answer/form inputs.

@dashouse dashouse merged commit 963a101 into master Jul 18, 2018
@dashouse
Copy link
Author

Merged thanks

@NickColley NickColley deleted the update-examples branch July 24, 2018 15:20
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