Improvements to Check your answers page

[selfthinker]

This makes markup, styling and content improvements to the 'Check your answers' page.

Markup changes

The markup was changed from a table to a description list.
We initially tried multiple variants which all looked and behaved the same and tested all of them in various assistive technology for potential accessibility issues:

• table (but not the one that is currently in the prototype kit)
• description list (dl)
• list (ul or ol)

We found that they all work nearly equally well and all have pros and cons. The main reason for deciding to use a description list was that it seems to be the most semantic in this case (although the others are not unsemantic). Although description lists have varying support in screen readers, the context made it still easy enough to understand and navigate.

Please note, the description list uses a div around each dt/dd grouping. That is currently invalid but it's in the HTML 5.2 Working Draft and it's supported in all the browsers (even IE6).

Visual changes

• Don't use the full page width if the items are short
• Remove the bold from the change link
• Make the key bold
• Make key and value wrap on smaller screens
• The widths automatically adjust to their content

Content changes

• Make the text more generic
• Include second section

[selfthinker]

FYI, I did it this way to allow other markup than a description list to use the same CSS. E.g. you could use a list or some divs instead and it would still look and behave the same.
I know this can also be done by giving every container and every item a class. I suspect that might be what people will ask me to change. I personally prefer it the way it is right now but would be fine with changing it if people felt strongly about it.

[NickColley]

Would be a good idea to apply classes rather than wildcards, this way people don't have to put aggressive overrides if they want to extend this pattern.

[selfthinker]

FYI, to get some context, these were the pros and cons we found with the three final contenders:

Table

• Pro: Data for monetary values might need table anyway
• Pro: Easier to navigate (for advanced screen reader users)
• Con: Tables generally difficult for less advanced screen reader users
• Con: Some argue it's not tabular data
• Con: If column headers are hidden, high risk of services not changing content

Description List

• Pro: Has most relevant semantics
• Pro: Easy to navigate for screen reader users
• Con: Various verbosity due to mixed screen reader support
• Con: Some screen readers don't differentiate between dt and dd
• Con: It's currently officially invalid markup
• Con: Unclear if having change link in second dd is semantic

List

• Pro: Easy to navigate for screen reader users
• Open: Choosing ordered or unordered list depends on usage

[adamliptrot-oc]

Just going off that list, I’d say (especially with the non-standard markup being used for definition lists) that standard lists are the winner.

[joelanman]

it does seem like List is a good option - it's simple, well known markup, and doesn't seem to have any downsides

[edwardhorsford]

On the other hand, it doesn't have the upsides of increased semantics that DLs have.

[joelanman]

the DL example on MDN does seem to be our use case: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dl#Metadata

[philsherry]

I tried this with some realistic test data from our service, using the actual questions that we have in our service.

[selfthinker]

@philsherry, remove the multiple-sections class from the dl. That should make it do the widths automatically. I tried to document that in the code. Do you think it should be on the page instead to make it clearer?

[philsherry]

This is with multiple-sections removed.

[edwardhorsford]

Hi @philsherry. We're going to update this PR to better deal with your situation of really long questions. Hold this space!

---

For your specific example, the questions could be much shorter. We recommend rewriting the information on the summary screen to be more concise. The page is replaying a summary of what the user has done, so doesn't need to duplicate the original questions. This has the added benefit of making the page easier to scan, and the visually hidden text in the change link shorter.

One of your questions appears to be a child of the first, so could be connected in the summary.

As an example:

Before

Question
How much total net profit does your business expect in the next 12 months, from the following activities?
Answer

• Money service business activities

£100,000 to £249,000

Ater

Question
Net profit expected in next 12 months
Answer
Total:
£100,000 to £249,000

By activity:
Money service business activities: £100,000 to £249,000

[etheya]

Hi Ed,
Can appreciate that you recommend that the information could be written in a shorter more concise way on the page, however having tried this originally with our users, it was very clear that this didn't work for them. Which is why we went back to the option of using the whole question on the page. Even with this in place the way we have OUR current Check Your Answers still enables the user to scan the information on the page I would argue even easier than the one you are suggesting.

Its not always possible to just use the activity here as a child either and added, as the user could do several activities so this wouldn't always work or lend itself to the suggestion that you have made.

I see no reason why the option that @philsherry has suggested could not be used in conjunction with the one currently on the table from yourselves. Its another viable options for lots of reasons depending on the content and context of the service.

Currently it feels like its either the way suggested by "the powers that be" (Mordor) and no other suggestion (The Fellowship) is taken on board. Surely there shouldn't just be one way of doing this, that would surely be to the detriment of doing what's best for the user which is all we are trying to achieve.

Can these sort of patterns not sit side by side depending on their usage?

[etheya]

We are not "precious" over the pattern... just looking at it as an alternative.

[NickColley]

I'm worried that if people try to use this pattern in pre-existing CSS it'll break since these classes are very generic.

I'd recommend namespacing the classes and making sure they're as robust as possible.

[selfthinker]

I thought of using > again, which would make them more robust as well. But you wouldn't like that, right?
Do we have any specific namespace that we use? On the one hand we have govuk-*, but on the other hand that in itself might not specific enough and, confusingly, we're not using it consistently.
I gave the SASS variable a prefix of cya (short for 'check your answers'). Would that be good? Or a combination (govuk-cya-*)?

[NickColley]

Might be worth holding off until there's some form of consensus from the new GOV.UK Frontend work.

(But wont block this)

[selfthinker]

We have four(ish) options, the choice of which is probably not super important for the prototype kit but will become more important for Elements:

Child combinator (what I had before)

.foo {
  > * {
  }
}

I personally find that quite resilient, especially because every child has to be styled in a certain way because the display: table children need to be display: table-row, etc.
(I wonder if I should even go as far and use :first-child for the question and :last-child for the change link.)
But I understand that people don't like this.

Simple classes (what I have now)

.foo {
  .bar {
  }
}

I agree with Nick that the class names are too generic.

Prefixed classes (BEM would also fit here, although the example below is not BEM)

.govuk-foo {
  .govuk-foo-bar {
  }
}

Probably where we're generally going? I personally don't like it but that's another story.

Simple classes with child combinator

.foo {
  > .bar {
  }
}

More resilient than just simple classes on their own. But not resilient enough to stop potentially wrongly global classes to have an effect.

[NickColley]

With BEM we can avoid the nesting also, keeping the CSS flat (in terms of specificity https://developer.mozilla.org/en/docs/Web/CSS/Specificity, https://csswizardry.com/2014/10/the-specificity-graph/) and ensure that this pattern will work without conflict when dropped into hostile codebases.

[selfthinker]

Thanks @philsherry for your example. When I put it into the prototype kit on Friday, it looked a little bit better. (The only thing knowingly different is that I didn't use a list in the third answer.)

@edwardhorsford and me then tinkered a bit with the widths and found that we were happy enough with it when we gave the question a width of 50%:

I had a comment in the CSS of my original code which suggested to override the width. But I have pushed a new version on Friday evening which now gives you the $cya-first-column-width variable to make it easier to override it. It is set to 30% by default. I have also removed the multiple-sections class as the effect of that can also be controlled via the $cya-first-column-width variable.

[edwardhorsford]

Suggest adding a comment with the recommendation for shorter questions. That way if a service goes away from the default, our recommendation is still in the comments.

Is it clear what we mean by single sections?

[joelanman]

I'm not sure what's meant by single sections

[selfthinker]

The default wouldn't change when services override it, it would still say 30%. But I can add a comment that the default is meant for short questions.

What else could we say instead of "single section"? Maybe "single set of questions"? Not sure... Any ideas?

[edwardhorsford]

How about this:

// ## Recommended column widths
// Single group of questions: auto
// Multiple groups of questions: 30% (mostly short questions) or 50% (mostly long questions)

[joelanman]

why should the column width change for a single group of questions?

[selfthinker]

Ideally I wanted to make all columns have an automatic width so that all content is flexible and you don't need to put in any constraints. That works the same way a table would work automatically. But as soon as you have a second group of questions, "designers" don't like the questions of both groups not vertically aligning. That's why you would need to give one column a fixed width.

[etheya]

Not sure about the “designers” part… I’m sure the “users” don’t like the questions of both groups not vertically aligning either as its far easier to read then they do.

…

On 29 Mar 2017, at 14:28, Anika Henke ***@***.***> wrote: @selfthinker commented on this pull request. In app/assets/sass/patterns/_check-your-answers.scss <#365 (comment)>: > @@ -1,14 +1,75 @@ + +// please adjust, depending on length of questions and answers and number of sections +$cya-first-column-width: 30% !default; +// recommended for single sections: auto +// recommended for longer questions: 50% Ideally I wanted to make all columns have an automatic width so that all content is flexible and you don't need to put in any constraints. That works the same way a table would work automatically. But as soon as you have a second group of questions, "designers" don't like the questions of both groups not vertically aligning. That's why you would need to give one column a fixed width. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#365 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEQ1IsJ9E1DfYIdqW-mkCxfbSU6GYbmBks5rqlx0gaJpZM4MkVoa>.

[selfthinker]

I just realised that using a SASS variable isn't as clever as I first thought. Because that means that you can only set it once per app!

[edwardhorsford]

@selfthinker Can you add screenshots of what it looks like with 30%, 50% and auto?

[selfthinker]

I have just pushed another version. There are three changes:

• It removes the SASS variable again and replaces it with a modifier class. The name is much better than the previous one, though. It's either cya-questions-short or cya-questions-long (or none).
• I reworded the comments. (They are in the HTML now.)
• I changed how the CSS is structured, half reverting to the previous version but also giving the classes a more distinct name. Not sure if that is the final version, @NickColley.

[edwardhorsford]

I don't think this is relevant any more. It would still be good to include a comment description of the widths we support in the scss file though - I expect the comment in the html is more likely to get removed at some point.

[selfthinker]

I had a smaller comment in the CSS further down. Do you think I should change anything about it?

[edwardhorsford]

Added a few comments on some minor things. I've already chatted to @selfthinker about these in person.

[edwardhorsford]

Question: do you think we should use the font-mixins to explicitly set the font size for check your answers?

[selfthinker]

No, I think our font mixins are used too often, which makes things less flexible and increases the CSS file size for no good reason.
If we had a variable for bold, that would be worth using, though. I couldn't find it, did I only dream about it?

[philsherry]

alphagov/govuk_elements#321

[edwardhorsford]

We moved to using visually-hidden a while back. Is now a good time to update this?

[edwardhorsford]

I've got a personal preference for the space after change to be inside the visually hidden span. Feels more correct that way.

[axemonkey]

We (Tax Tribunals project, MOJ Digital) implemented the <dl> pattern in our service which is in private beta, then got the DAC in Wales to accessibility check our service. They marked this is a high priority fail, because of the invalid HTML nesting, i.e. the <div> wrapper being a child element of <dl>.

This is the quote from their report:

Screen readers have a specific way of announcing definition lists. When such lists are not properly marked up it may lead to the information becoming confusing or inaccurate to screen reader users when read back.

Their suggested fix is, of course, to remove the <div> which would break the CSS it is there to provide.

[gemmaleigh]

Could we keep the units consistent here and use em(5,19) - rather than the $gutter variable.

[selfthinker]

I've changed it now to em(4, 19) (not 5 because it was only 5px due to the $gutter but should really have been 4px).

I've also rewritten the structure so that the@media queries are closer to their mobile equivalents as you mentioned (verbally) earlier.

[selfthinker]

@axemonkey, to sum up what we said in the cross-government Slack channel when you brought this up:

• They are generally right in highlighting invalid markup as that can lead to less accessible markup, or in their own words, it may lead to it.
• But as we were aware of that, we tested it very vigorously in various screen reader and browser combinations and couldn't find any issues. (In one case the result was even better than without a div.)
• We should ideally document what exactly we have tested and what the outcome was with a div versus without a div.
• It would be good to know if DAC actually found any issues or if this is a standard sentence they put in when they find any invalid markup. If no-one finds any issues, I would argue this is as accessible as it can be and should only be a warning or a "pass with comment" but not a "high priority fail".
• It would also be interesting to know which HTML validator DAC was using. The current W3C validator has no problem with the div and the validation passes.

I will try to document our specific outcomes soon.

[gemmaleigh]

Personally, I'd prefer if we used classes - rather than wildcards as @NickColley mentioned, I think it makes it easier to read.

I'd also prefer it if each media query block sat inside the content it related to, so it is clear where overrides are occurring for desktop.

For example:

.cya-change {
  text-align: right;
  position: absolute;
  top: 0;
  right: 0;
  
  @include media(desktop) {
    position: static;
    padding-right: 0;
  }
 }

This is really where I'd prefer the nesting occurs, if at all.

But these are small changes to the scss we can make later, if others also agree they are useful. I won't hold this up any longer.

[selfthinker]

Thanks @gemmaleigh! I can make the media queries more granular. I'll do that today or tomorrow.
As to the classes over wildcards, when I changed that before because Nick requested it, he wasn't happy with my class names. As we don't have any guidance for class names, this is likely to go back and forth quite a lot if I change it again. That's why I chose to go with what I prefer instead of continuously guessing what other people prefer. I'm happy to use classes as soon as we have established some rules around them.

[selfthinker]

@gemmaleigh, I have made the media queries more granular now. I only left the section with .cya-questions-short and .cya-questions-long as it was because that's difficult to split apart.