Bump toolkit and bring in new radios/checkboxes #302

pcraig3 · 2017-02-07T15:39:18Z

This pull request:

bumps the govuk_frontend_toolkit
bumps the govuk_template, removes external-link styles
brings in styles for the new radios and checkboxes first popularised by @robinwhittleton of govuk_elements fame
adds 'role="button"` to links that are buttons
bolds question headings

new radios and checkboxes

New radios and checkboxes look nice, and I've removed most of our custom spacing rules as those in govuk_elements seem to work better with the new elements.
We also had a problem passing values to prefill our inputs with but that's been fixed.

external links

Removing our external-link styles to keep consistent with the govuk toolkit, but we're keeping the macro because our apps still rely on the markup.

phase banners

Stopped passing a phase to the phase-banner() mixin. Banners are now blue, but no visible changes to sizing or spacing.

bold question headings

This is what govuk_elements are doing and now it's what we are doing.

Worth mentioning that I ran wraith against all the pages after updating the govuk_frontend toolkit and then only real visual changes I found were to do with external links and the beta banners.

gifs

old	new

screenshots

old: what the selection buttons looked looked like before
old spacing: what the new elements looked like with digitalmarketplace-specific spacing preserved
new spacing: what the new elements look like with our hyper-rationalised vertical spacing

The differences between the old spacing and the new spacing are:

form elements are more tightly grouped
the labels have been moved away from the input elements (this is particularly noticeable when the focus border is applied)
the spacing between all meta-elements (question_advice, hints, optional tags, validation) and the questions themselves is now equal

radios

old	old spacing	new spacing

checkboxes with hint and question advice

old	old spacing	new spacing

new spacing

All distances are now equal.

robinwhittleton

Obviously I don’t have a working copy of DM, but on the face of it this looks reasonable.

robinwhittleton · 2017-02-07T16:47:23Z

pages_builder/assets/javascripts/onready.js

-      'focusedClass' : 'selection-button-focused',
-      'selectedClass' : 'selection-button-selected'
-    });
+    var selectionButtons = new GOVUK.SelectionButtons('label.selection-button');


Usually the selection button JS expects to apply to the input, not the label. Is this working correctly?

Good spot, we've done so in our apps so looks like this was always wrong.

Very refreshing getting some other 👁 's on this code btw, thanks!

robinwhittleton · 2017-02-07T17:00:27Z

toolkit/templates/external-link.html

-  <a href="{{ link }}" rel="external" {% if target %}target="{{ target }}"{% endif %}>{{ text }}</a>
+  <a
+    href="{{ link }}"
+    rel="external{% if target=='_blank' %} noopener noreferrer{% endif %}"


tombye · 2017-02-09T14:27:13Z

pages_builder/assets/javascripts/onready.js

-      'focusedClass' : 'selection-button-focused',
-      'selectedClass' : 'selection-button-selected'
-    });
+    var selectionButtons = new GOVUK.SelectionButtons('label.selection-button');


Good spot, we've done so in our apps so looks like this was always wrong.

Very refreshing getting some other 👁 's on this code btw, thanks!

tombye · 2017-02-09T14:29:27Z

CHANGELOG.md

+find(
+    :xpath,
+    "//*[contains(text(), 'Service status')]/following-sibling::*[@class='selection-button selection-button-selected'][text()]"
+  ).text().should have_content("#{service_status}")


This mentioning the XPath needed to change any associated functional tests is excellent.

tombye · 2017-02-09T14:32:54Z

CHANGELOG.md

+```ruby
+find(
+    :xpath,
+    "//*[contains(text(), 'Service status')]/following-sibling::*[@class='selection-button selected'][text()]"


Probably worth swapping [@class='selection-button selected'] for [@contains(@class, 'selection-button selected')] so it can cope with other strings being present in the class attribute.

tombye · 2017-02-09T15:09:09Z

toolkit/scss/forms/_selection-buttons.scss

-.js-enabled .selection-button-focused input:focus {
-  outline: none;
+// To stack horizontally, use .inline on parent, to sit block labels next to each other
+.inline .block-label {


I think this is actually the same block as line 143 as we use selection-button instead of block-label in our code.

I think the fact this duplication wasn't obvious points to the .inline & selector being unclear. We should combine both of them with a selector of .inline .selection-button, or maybe even fieldset.inline .selection-button for even more obviousness.

Yup, makes sense.
Lose terseness but gain clarity.

Read a blog post by Harry Roberts the other day and that seems to be one of his ideas.

tombye · 2017-02-10T12:59:42Z

toolkit/scss/forms/_questions.scss

@@ -12,6 +12,11 @@
  .hint {
    margin-bottom: 0;
  }
+
+  // This preserves the vertical margins set on the .question-headings
+  legend {


Can we check this with @ralph-hawkins? This will add 10px to the spacing between legend and first field so we should make sure we want this.

This doesn't mean I'm against this exact change, it's just worth asking if we need to also adjust the padding and line-height so the behaviour of the CSS is correct but the visuals stay the same.

Yeah, sure. Good point.

The bottom margin is already there but it's getting ignored, which's means that we should either
i) add this so that it shows up, or
ii) remove the margin

Having the rule and not using it seems like the wrong thing to be doing.

For the record, this was resolved by @pcraig3 working with @ralph-hawkins to make the vertical spacing for all questions consistent, whatever elements are present about the field.

tombye

Overall this looks great and very thorough. I've a few comments but nothing too big. Once they're sorted and it's ready to merge 👍

tombye · 2017-02-20T15:59:40Z

toolkit/scss/forms/_questions.scss

@@ -21,9 +40,10 @@

 .question-heading,
 %question-heading {
-  display: block;
  @include heading-24;


We should replace the @include heading-24 and the font-weight: 700 with bold-24 to match the GOVUK Elements style we want to use. I think we'll need the margins from that style too as heading-24 brings some in with it.

tombye · 2017-02-20T16:28:06Z

toolkit/scss/forms/_questions.scss

 @import "shared_scss/_dmspeak.scss";
 @import "shared_placeholders/_visuallyhidden.scss";

+
+// Spacing rules based on those found in govuk elements selection buttons


It might be good to move this into its own placeholder file, just incase we end up wanting to import _questions.scss into anything in future (in which case the selectors produced will appear in all those places).

tombye · 2017-02-20T16:40:15Z

toolkit/templates/forms/selection-buttons.html

    <legend>
-      <span class="question-heading{% if hint or question_advice %} question-heading-with-hint{% endif %}">
+      <span class="question-heading{% if hint_underneath %} question-heading-with-hint-underneath{% endif %}">


Does the `question-heading-with-hint-underneath class exist? I can't find it in this repo (or this pull request).

The work done on vertical spacing (mentioned elsewhere on this PR) removed the need for question-headings-with-hint.

tombye · 2017-02-20T16:45:37Z

toolkit/templates/forms/selection-buttons.html

        />
      </label>
-      <label class="selection-button selection-button-boolean" for="input-{{ name }}-no">
+      {% set default_no = true if value is sameas false %}
+      <label class="selection-button selection-button-radio {% if default_no %} selected{% endif %}" for="input-{{ name }}-no">


This doesn't need a space before the {% if default_no %} opening block.

tombye · 2017-02-20T16:48:12Z

toolkit/templates/forms/textbox.html

@@ -28,7 +28,7 @@
      {% endif %}
    </label>
    {% if optional %}
-      <span class="question-optional" style="margin-top: 10px;">


Nice bit of tidying up this.

According to their changelog, they're up to 5.1.1, but on npm they're only at 5.0.3.

This has been removed from the govuk_frontend_toolkit so it was breaking our imports. Just going to remove them entirely. Removing external links in the govuk_frontend_toolkit: alphagov/govuk_frontend_toolkit#293

@tombye

Apparently, _blank links are the most underestimated vulnerability ever. Source: https://www.jitbit.com/alexblog/256-targetblank---the-most-underestimated-vulnerability-ever/ I'm not convinced, but, as @tombye pointed out, we don't lose anything. Adding them where external links have a target="_blank" *** Also worth mentioning that even though I've removed all of the external-link styles from the toolkit, our frontend apps (at this point, supplier and admin) are using the external-link template and so rely on its markup. Probably worth removing the surrounding div eventually (maybe removing the pattern altogether) but not now.

We're following the lead set by govuk_elements. No more background, but a bigger hit area for checkboxes and radio buttons. Also needed a change to our `selected` classname because that's how these new inputs are driven. Custom radios / checkboxes, in govuk_elements: alphagov/govuk_elements#296

@tombye

Javascript doesn't know how to unselect our elements unless they're contained within a form. Since @tombye added a `surrounding_html` key, now we can add this to our examples.

Similar to the radios and checkboxes, govuk_elements have added new styles that increase the contrast of our form elements. Worth noting that we're explicitly _not_ bringing in spacing-related styles because we're Digital Marketplace and we do things differently here. Custom radios / checkboxes, in govuk_elements: alphagov/govuk_elements#296

Updating to the new govuk_element styles broke our logic for passing in default values, because we need to add a class to the `label` surrounding the input now for it to display properly.

Similar to the previous commit. The new govuk_elements styles want a class on the label surrounding the `<input>` to display properly in a browser. Refactored the logic, which meant creating the label only after we know if we have a set of radios or checkboxes.

Our IRL assurance questions don't have question marks.

The `name` attribute of our examples becomes the id of the generated element. Since we copy+paste them loads, we have tons of repeated ids on our example pages. This is a bit of a brute-force way to manage them, but at least it's clear where they're coming from. I considered adding some incrementing logic somewhere in the `generate_pages` file but, given how much I don't like going in there to make changes, it's unlikely anyone else would want to go in there looking for some magic incrementing logic either.

The govuk_frontend_toolkit has outlawed passing in the 'phase' to the phase-banner components. Colour is now always the same, and it is now just the text inside the phase tag that changes. This represents a breaking change for our frontend apps. Make alpha, beta and discovery colours $govuk-blue, in govuk_frontend_toolkit: alphagov/govuk_frontend_toolkit#370

Removes all that lingering external-link stuff.

There was a larger discussion around this in the `govuk_elements` repo. Source: alphagov/govuk_elements#404 for those interested. TL;DR focus border was 5px around checkboxes/radios but 3px around textboxes. Make the former thinner for consistency.

Earlier, when porting over the govuk_elements' styles, we ignored their spacing rules because we do things differently here at the Digital Marketplace doncha know. However, our spacing didn't really look that great: - questions with question-advice and hints and everything would look really squished - side-by-side radios didn't really have enough space between them to unambiguously associate the label with its taget input Brought in all the govuk_elements spacing styles instead of ours and they look well better, I think.

Amazingly, the bottom margin we add to all question headings has been, up to now, not applied. It turns out that without a `block_formatting_context`, block-level children of parent elements will not respect margins added to the question heading. Setting an explicit height has done the trick. This change means lots of changes to our form elements, but I think it's a big improvement. More on collapsing margins: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Box_Model/Mastering_margin_collapsing More on block formatting contexts: https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Block_formatting_context

`.js-enabled .js-hidden` is defined in the wider govuk styles and so we don't need to redefine it in our app.

- use our macro instead of HTML for the form field - add a page title to an example that was missing it - add more answers with large labels - make sure there is one question with only a hint and another with only question_advice

Had a few issues where leftover files weren't being removed from the copied location. Now, we make sure to delete the auto-generated stuff every time.

Grabbed the styles being used by the govuk_elements conditionally-revealing content. Notably: - less spacing between label and input field - regular-sized font weight for label (not bold) - less space between the inital question field and the revealed content

We've decided all question headings should be bold now. https://medium.com/public-innovators-network/what-if-boldness-were-an-explicit-value-of-the-civil-service-3df6a3d2d008 Make question-headings display: block to remain consistent with heading styles in the govuk_frontend_toolkit.

The actual text in our text fields was closer to the top (and bottom) border than the text in our textareas. Stopped setting an explicit height value on our text fields and instead bumped the vertical padding. Means our text fields look as before and our textareas are a bit more spacious/luxurious.

@ralph-hawkins

Our questions can be as simple as: - question - input Or they can be as complicated as: - question - validation error message - 'optional' tag - question advice - hint - input - hint-underneath Or any combination of these things. Over time, we've accumulated different paddings and margins for each new element (and some elements gain/lose their spacing depending on the element that precedes them). So it's gradually become a bit of a mess, if we're honest. When bringing in the new buttons, @ralph-hawkins pointed out our haphazard vertical spacing, and suggested we come up with a uniform value and then implement it partout. We decided to give the same spacing separatingthe text of vertically-aligned radios and checkboxes to all of our question meta-fields, except on mobile, where the text gets smaller and the large padding becomes too enormous. I've created a SASS partial based on the spacing used for the new selection buttons, and @Extended it in all of our question meta values, killing almost all custom spacing along the way. This makes for a pretty big change to all of our form elements, but the benefits are that we now have a uniform approach that makes our existing questions look cleaner and will allow us too easily add more fields.

This class is no longer targeted by any specific CSS rule, so we're free to ditch it.

Another of the changes pulled in in the latest template is that links with a role="button" can be targeted by a script that will treat them the same as buttons. This means you can press the space bar when they're focused and it should register a click. Frontend apps will have to set up their javascript independently, but the button role will be in the markup from now on. Source: alphagov/govuk_frontend_toolkit#297

Our new, authoritarian spacing rules use 7px to p padding, 7px bottom padding, and 10px bottom margin, whi ch is 4px of space overall. Having a marging between questions of only 30px (since vertical margins collapse) was just not enough. Hopefully this resolves our issue, although we can tweak it if we need to. Also add the same spacing to validation questions.

tombye · 2017-02-22T12:03:01Z

Once it's good to be merged, 👍

pcraig3 force-pushed the pc-import-improved-inputs branch from 8ee84fd to ffd0844 Compare February 7, 2017 15:45

pcraig3 changed the title ~~Pc import improved inputs~~ [WIP] Bump toolkit and bring in new radios/checkboxes Feb 7, 2017

robinwhittleton reviewed Feb 7, 2017

View reviewed changes

pcraig3 force-pushed the pc-import-improved-inputs branch from 5adc8c2 to eac1ac0 Compare February 8, 2017 15:24

tombye reviewed Feb 10, 2017

View reviewed changes

pcraig3 force-pushed the pc-import-improved-inputs branch 3 times, most recently from e5d2e3d to 1b1b753 Compare February 10, 2017 11:11

tombye reviewed Feb 10, 2017

View reviewed changes

pcraig3 force-pushed the pc-import-improved-inputs branch from e7c96b2 to d3ef7dd Compare February 13, 2017 15:36

pcraig3 changed the title ~~[WIP] Bump toolkit and bring in new radios/checkboxes~~ Bump toolkit and bring in new radios/checkboxes Feb 13, 2017

pcraig3 force-pushed the pc-import-improved-inputs branch 7 times, most recently from 3677b58 to 008ee1c Compare February 16, 2017 13:22

pcraig3 mentioned this pull request Feb 16, 2017

[WIP] Bring in new toolkit with new input fields Crown-Commercial-Service/digitalmarketplace-buyer-frontend#444

Closed

tombye reviewed Feb 20, 2017

View reviewed changes

pcraig3 added 10 commits February 22, 2017 01:11

Bump govuk_frontend_toolkit to 5.0.3

fb3da9d

According to their changelog, they're up to 5.1.1, but on npm they're only at 5.0.3.

Remove external-link styles

6a89a7d

This has been removed from the govuk_frontend_toolkit so it was breaking our imports. Just going to remove them entirely. Removing external links in the govuk_frontend_toolkit: alphagov/govuk_frontend_toolkit#293

Wrap checkbox and radio examples in a form

5722dc4

Javascript doesn't know how to unselect our elements unless they're contained within a form. Since @tombye added a `surrounding_html` key, now we can add this to our examples.

Correctly pass defaults to boolean radios

d606786

Updating to the new govuk_element styles broke our logic for passing in default values, because we need to add a class to the `label` surrounding the input now for it to display properly.

Remove question marks from example questions

166b869

Our IRL assurance questions don't have question marks.

pcraig3 added 20 commits February 22, 2017 01:14

Update govuk_template

bb7db9a

Removes all that lingering external-link stuff.

Remove toolkit-specific .js-hidden

9f38237

`.js-enabled .js-hidden` is defined in the wider govuk styles and so we don't need to redefine it in our app.

Better examples in yml file

acdd03d

- use our macro instead of HTML for the form field - add a page title to an example that was missing it - add more answers with large labels - make sure there is one question with only a hint and another with only question_advice

Delete unused code in pages_builder/public

faffe91

Add clean step to npm

d98a97f

Had a few issues where leftover files weren't being removed from the copied location. Now, we make sure to delete the auto-generated stuff every time.

Remove .question-heading-with-hint

28ff7f1

This class is no longer targeted by any specific CSS rule, so we're free to ditch it.

Bump version to 20.0.0

4b29edc

Update changelog with breaking changes

ecea647

Have some self-respect, jesus

09d5f7c

Move question vertical spacing to _placeholders.scss

b135157

pcraig3 force-pushed the pc-import-improved-inputs branch from 008ee1c to b135157 Compare February 22, 2017 10:03

pcraig3 merged commit c721d04 into master Feb 22, 2017

pcraig3 deleted the pc-import-improved-inputs branch February 22, 2017 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump toolkit and bring in new radios/checkboxes #302

Bump toolkit and bring in new radios/checkboxes #302

pcraig3 commented Feb 7, 2017 •

edited

Loading

robinwhittleton left a comment

robinwhittleton Feb 7, 2017

tombye Feb 9, 2017

robinwhittleton Feb 7, 2017

tombye Feb 9, 2017

tombye Feb 9, 2017

tombye Feb 9, 2017

tombye Feb 9, 2017

pcraig3 Feb 10, 2017

tombye Feb 10, 2017

pcraig3 Feb 10, 2017

tombye Feb 22, 2017

tombye left a comment

tombye Feb 20, 2017

tombye Feb 20, 2017

tombye Feb 20, 2017

tombye Feb 22, 2017

tombye Feb 20, 2017

tombye Feb 20, 2017

tombye commented Feb 22, 2017

Bump toolkit and bring in new radios/checkboxes #302

Bump toolkit and bring in new radios/checkboxes #302

Conversation

pcraig3 commented Feb 7, 2017 • edited Loading

new radios and checkboxes

external links

phase banners

bold question headings

gifs

screenshots

radios

checkboxes with hint and question advice

new spacing

robinwhittleton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombye left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombye commented Feb 22, 2017

pcraig3 commented Feb 7, 2017 •

edited

Loading