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 check answers and extended grid #692

Merged
merged 2 commits into from
Jan 16, 2019

Conversation

dashouse
Copy link

This PR adds:

A coded example of the 'Check answers' pattern using the 'Summary list' component and the desktop specific grid.

Documents the extended grid on the layout page (needs a review / chat with @amyhupe)

@dashouse
Copy link
Author

This needs a <form> tag added to the Check answers example

@dashouse dashouse force-pushed the add-check-answers-and-extended-grid branch from 3a4cb49 to 3406ee5 Compare January 14, 2019 08:34
@govuk-design-system-ci
Copy link
Collaborator

govuk-design-system-ci commented Jan 14, 2019

You can preview this change here:

Built with commit 827270c

https://deploy-preview-692--govuk-design-system-preview.netlify.com

@36degrees
Copy link
Contributor

Totally non-blocking, and I'm aware this is how it was in the existing pattern, but the more I look at it the stranger I find it it is that there are entries for both 'Contact information' and 'Contact details'…

Surely that's just two different ways of saying the same thing?

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

This looks ace @dashouse 👌

Picked up on a couple of minor things that could do with being addressed, and I know this needs a review from @amyhupe too.

src/patterns/check-answers/default/index.njk Outdated Show resolved Hide resolved
src/styles/layout/desktop/index.njk Outdated Show resolved Hide resolved
src/styles/layout/tablet-desktop/index.njk Outdated Show resolved Hide resolved
@36degrees 36degrees requested a review from amyhupe January 14, 2019 10:25
@amyhupe
Copy link
Contributor

amyhupe commented Jan 14, 2019

I think this looks really good – nice one @dashouse

None of my comments are blocking and all relate to things which haven't been introduced by this PR, but:

  1. I agree with Ollie's comment about changing the button text in the example to "Accept and send" to match the button component guidance.
  2. Looking at it now, I think the Licence period entry in the example being the only one with non-realistic example text is a bit unhelpful, is it possible to swap this for a realistic answer?
  3. We should remove all the upper case "C"s from mid-sentence mentions of check answers as we no longer follow this convention.

That's all from me.

Thanks

@dashouse dashouse force-pushed the add-check-answers-and-extended-grid branch from 3406ee5 to 827270c Compare January 14, 2019 14:49
@dashouse dashouse merged commit 0cb2a5f into master Jan 16, 2019
@NickColley NickColley deleted the add-check-answers-and-extended-grid branch January 16, 2019 15:10
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.

4 participants