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 step by step navigation #539

Merged
merged 6 commits into from
Nov 1, 2018
Merged

Add step by step navigation #539

merged 6 commits into from
Nov 1, 2018

Conversation

joelanman
Copy link
Contributor

@joelanman joelanman commented Jul 5, 2018

This work adds a Step by step navigation page to the template pages in the Prototype Kit.

This means that a service can consider and prototype Step by step navigation as part of their service. It's a pattern we've found to be very successful and we'd like more teams to try it.

This is not an in-service pattern. Similar to Start pages, it would live on gov.uk publishing platform. If teams found a user need for it, they would need to work with the GOV.UK publishing team to get a page built or changed.

Since services should not be implementing this pattern themselves, this code is for prototyping, not production.

This is based on GOV.UK Publishing Components code, re-written for Nunjucks and GOV.UK Frontend

Original files:

Preview with the 'active deployment' link further down

@kr8n3r
Copy link

kr8n3r commented Jul 6, 2018

would change gem-c to app-

@dashouse
Copy link

dashouse commented Jul 6, 2018

In terms of the CSS specifically, there's a few things we would do differently if this was a GOV.UK Frontend component and it doesn't really follow some of the design conventions we have.

I don't know how much we care about that considering how it's used... but we wouldn't want people ripping this CSS if they're making something similar looking?

@joelanman
Copy link
Contributor Author

joelanman commented Jul 6, 2018

@igloosi yup, agree, will have a bash at doing that

@dashouse agree we can tidy up, are there any specific changes you'd recommend? On the other hand I think we need to be clear that this pattern and Start pages are for prototyping only and not for copying and using in production.

@dashouse
Copy link

dashouse commented Jul 6, 2018

I think some of it is just trying to use the GOV.UK Frontend font size and spacing mixins and potentially removing the things that are not needed (I'm not sure the font-family needs to be defined in the block__elements in this way for us - must be for publishing frontend).

Other things are not so simple...if we were going to make this in GOV.UK Frontend I think we'd start with the base type styles, line-heights and only override if necessary...we've tended to try and avoid collapsing / negative margins like below

screen shot 2018-07-06 at 10 08 50

I'm not too worried about people copying and pasting to get "step-by-step" I'm more worried about people copy / pasting to get "an accordion with a tube map" or a "bordered section with list" as these are things people want that we don't currently have code for...

@joelanman joelanman force-pushed the step-by-step branch 2 times, most recently from 15cfb34 to dc0b1f8 Compare July 6, 2018 11:24
@joelanman
Copy link
Contributor Author

Now using app- throughout instead of gem-c-

@joelanman joelanman force-pushed the step-by-step branch 4 times, most recently from d1359c6 to 109e7c9 Compare July 6, 2018 16:41
@joelanman joelanman force-pushed the step-by-step branch 4 times, most recently from 5e7310c to db69311 Compare October 17, 2018 10:48
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-prototype-kit-pr-539 October 17, 2018 10:48 Inactive
@joelanman joelanman temporarily deployed to govuk-prototype-kit-pr-539 October 17, 2018 13:54 Inactive
@joelanman joelanman temporarily deployed to govuk-prototype-kit-pr-539 October 17, 2018 17:01 Inactive
@joelanman joelanman temporarily deployed to govuk-prototype-kit-pr-539 October 18, 2018 11:26 Inactive
@joelanman joelanman changed the title FOR DISCUSSION - Add step by step navigation Add step by step navigation Oct 18, 2018
z-index: 6;
bottom: 0;
left: 0;
margin-left: 6.5px;
Copy link

Choose a reason for hiding this comment

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

half a pixel?

@joelanman
Copy link
Contributor Author

I'm not totally sure about using govuk-spacing() throughout - isn't that responsive? The original measurements are often not responsive

@NickColley
Copy link
Contributor

Is there a benefit in keeping the files as close as the originals as possible? Otherwise whenever the GOV.UK team update the component we'll have to rewrite it again?

@joelanman
Copy link
Contributor Author

@NickColley it's tricky, as you say keeping it close makes it easier to maintain. However, we can't use the original templating language, as it's Ruby. We can't use the original Sass unless we also import the whole stack it depends on. Using the original code also makes it harder for our users to update the page with the details for their service, as the conventions are different to Frontend.

There are also features that we don't want - for example tracking activity in Google Analytics.

So, I'm aiming for a compromise that is close to the original approach, but uses our stack and conventions. This does make maintenance harder, but the Step by step team says they don't expect any major updates. In addition, I don't think this pattern needs to be a perfect copy - this is not a pattern teams will implement in their service, at the end of the day they will be using the original pages on GOV.UK. The value is really just in letting teams prototype their service within an end to end journey.

@NickColley
Copy link
Contributor

@joelanman yeah I think if the team doesnt expect that much change to the pattern then it seems fine

@joelanman joelanman temporarily deployed to govuk-prototype-kit-pr-539 October 19, 2018 16:59 Inactive
@joelanman
Copy link
Contributor Author

@igloosi I think I might go back to 'magic numbers' - our spacing scale only goes up to 9 (60px) and there are some larger values here, there are also quite a few values that aren't in our scale (3px, 8px, 16px). Might be cleaner, easier to read and update from the original files if they're all magic numbers rather than some using spacing and some not.

@dashouse
Copy link

@joelanman yeah, it's going to be really hard for you to try and simply swap values without changing the way the spacing has been applied on the GOV.UK component generally.

I think a lot of the numbers that don't appear on the scale come from parts of the component that we'd build differently if we were building as a GOV.UK Frontend component.

For example we would probably take a look at the negative margin here:

screen shot 2018-10-22 at 08 52 43

The 8px and the 3px come from a container and content of the "show all" button. Which could be combined into a govuk-spacing(1) on top and bottom on the button and removing the spacing on the container all together.

screen shot 2018-10-22 at 08 53 59

screen shot 2018-10-22 at 08 53 55

p.s. There's a couple of references to govuk-spacing(16) in the scss file which is breaking the build.

@joelanman
Copy link
Contributor Author

OK - going to rewrite back to magic numbers, and have a warning at the top that this is not in GOV Frontend style.

@joelanman
Copy link
Contributor Author

We've decided not to go with the 'GOV.UK Publishing pattern' label on the Design System, so we should probably drop it here too. However that leaves us with no guidance to say these pages are different to the others. We could add a link for each page to see the guidance on the Design System.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-prototype-kit-pr-539 October 25, 2018 09:45 Inactive
@joelanman joelanman temporarily deployed to govuk-prototype-kit-pr-539 October 25, 2018 18:01 Inactive
@joelanman
Copy link
Contributor Author

All changes done - could probably do with one last code review

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.

based on comments we're happy with this as is

@kr8n3r
Copy link

kr8n3r commented Oct 26, 2018

@joelanman missing changelog

@joelanman joelanman merged commit 772626f into master Nov 1, 2018
@joelanman joelanman deleted the step-by-step branch November 1, 2018 14:13
@kr8n3r kr8n3r mentioned this pull request Nov 1, 2018
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.

5 participants