ONSdigital / sdg-indicators

Development website for collecting and disseminating UK data for the Sustainable Development Goal global indicators.
https://ONSdigital.github.io/sdg-indicators/
37 stars 66 forks source link

Cookie consent form with GDS #3532

Closed brockfanning closed 3 years ago

brockfanning commented 3 years ago

This is a replacement for #3524 (which got closed anyway), using the the GDS assets more directly.

This depends on open-sdg/open-sdg#1235 so this PR points there in remote_theme.

Some notes on the PR:

LucyGwilliamAdmin commented 3 years ago

@phillipgardiner @brockfanning feature branch: http://uk-sdg-feature-branches.s3-website.eu-west-2.amazonaws.com/feature-site-cookies-gds-npm/

brockfanning commented 3 years ago

@LucyGwilliamAdmin There's an issue with the downloading of the font - I didn't think about the different subfolder on the feature branch. Can you make this change in your branch? Change this line to:

$govuk-assets-path: "/feature-site-cookies-gds-npm/assets/";
LucyGwilliamAdmin commented 3 years ago

@brockfanning, thanks I've just made that change - I think it's downloading now?

brockfanning commented 3 years ago

Yep, looks good.

phillipgardiner commented 3 years ago

@brockfanning @LucyGwilliamAdmin @AnnCorp I'm very pro using the transport font. However this would require some tweaking to the font sizes used for certain apsects of the site. For example using a 16px for the main body text is too small, this would need to be at least 18px. There are others too, again GDS do have recommendations of what to use. If we go ahead with this perhaps we need another tickets font size changes, I could then work out what needs to change to what?

phillipgardiner commented 3 years ago

@brockfanning @LucyGwilliamAdmin @AnnCorp Looking through the rest of the cookies pages etc, I think we are going to have to do my above comment (change our body text to use 'gov-body') As we are now using standard GDS components in some instances, they automatically set the font size for these elements (which is 19px, govuk-body). It therefore then looks odd when we have our body text size 16px and then the GDS component uses 19px as standard.

phillipgardiner commented 3 years ago

@brockfanning is the cookie bits finished and ready for me to take a look at properly now and provide any feedback?

brockfanning commented 3 years ago

@phillipgardiner That makes sense. I can look into adding that body class, as that seems like it should be fairly easy. Also yes, the cookie bits are ready to review in the meantime.

phillipgardiner commented 3 years ago

@brockfanning this is looking REALLY good, couple of things I've spotted.

  1. In both the confirmation messages the word 'additional' needs replacing with 'analytics'
  2. When saving your cookie settings the green confirmation box should appear above the H1 page title
  3. Not sure how much control we have verses it being GDS controlled but the green confirmation box seems too wide and the font too heavy. GDS guidance for cookies confirmation seems to be an example only. I personally don't like the wording used and would prefer if we could use this copy (used on actual gov.uk site) as it ties in with the button labelling better Screenshot (201) We obviously don't need the middle sentence though.
  4. Can we increase the white space between the last paragraph and the 'save cookies settings' buttons (approx 32 top padding)
  5. There exists a couple of content changes required on the cookies page in order to match the Cookies_Policy_v4.docx document. I can go through and list them out if you wish but don't know if its easier to just copy and paste it all across again? If so please ensure the cookie descriptions are copied from this document too as some have been adjusted from whats on the hotjar website.
  6. The privacy policy link on the cookie page will need to link to our privacy policy (not sure this has been done yet though?)
  7. There also exists a few problems when you select high contrast, primarily, the cookie banner text disappears as becomes white text on grey background and the radio buttons on the cookie page are invisible (maybe this is because GDS do not account for high contrast settings?) 7, Finally, as discussed the body text will also require changing to 19px in order to look correct on the cookie page (but we've already mentioned this above). I also believe a couple of other text sizes will need updating, primarily the navigation bar to 19px and 'take part in user research' button text to 16px

That's it I think, good work :)

phillipgardiner commented 3 years ago

@brockfanning sorry one more. When javascript is turned off and a user clicks the cookie link in the footer they should be taken to an alternative page, as show on page 3 of this document Cookies_Page_v3.pdf

brockfanning commented 3 years ago

@phillipgardiner Regarding no. 5, since the code needs to be HTML, it's not possible to copy/paste the whole thing in bulk. I would need to copy/paste each individual paragraph/cell/etc. So if it's not too much trouble then it might be more efficient if you could point out the specific changes needed - or summarize them (eg, "all the descriptions of the hotjar cookies", etc.). I started by copying what is already on the published cookies page.

Regarding the font sizes, I misunderstood before. I was thinking we could add a single class to the <body> element, but now I see that we would need to add classes to individual elements. (This is what I was referring to before about needing to "rewrite all the markup".) I think for now it will actually be simplest to mimic the GDS font sizes element-by-element, using custom CSS. I can start with the ones you've mentioned already. By the way, I also wanted to make clear: if you want to split out the font change to another ticket, that is perfectly doable. Either way is fine with me -- it's totally up to you. If we did split it out, then we would need to first complete this cookies ticket before going on to the font change.

Regarding the no-javascript content for the cookies page, that is actually quite tricky. What would be much simpler would be to display the no-javascript content at the top, in addition to the normal content. Ie:

My page heading

My no-javascript content...

My normal content...

It's much trickier to actually hide content when javascript is turned off:

My page heading

My no-javascript content...

I say "tricky" because it's not impossible, but it definitely increases the complexity, which increases your maintenance burden. So I wanted to alert you of this in case the simpler approach is acceptable.

phillipgardiner commented 3 years ago
  1. In opening paragraph replace the number '2' with the word 'two'
  2. _gat cookie description, change to 'Used to manage the rate at which page view requests are made'
  3. _hjRecordingLastActivity cookie, change 'sessionStorage' to 'session storage'
  4. Remove the question 'Do you want to accept analytics cookies?' note: may need to have this in the background somehow (aria label?) for screen readers though?
  5. Essential cookies paragraph change too: 'Essential cookies keep your information secure, and do things like remember your preferences and the choices you make whilst you use the website. They always need to be on.'
  6. Swap the order of the contrast and cookie lines in the table, so cookie-settings is first
  7. I'm guessing we are using a cookie called cookie_settings to cover both of these instances?:
    • cookies_policy | Saves your cookie consent settings | 1 year
    • cookies_preferences_set | Lets us know that you’ve saved your cookie consent settings | 1 year

That's all the content changes, I will update you with font feedback shortly - thanks

phillipgardiner commented 3 years ago

@brockfanning basing on mimicking the GDS font sizes element-by-element. On the basis of getting close but still keeping the hierarchy of our pages these are the ones so far. There is quite a few! I'd need to have another look once done to see if I missed anything. Perhaps it is worth discussing on the tech call tomorrow this approach verses delaying the use of the transport font? (I'm not sure how much effort these listed changes will be, against needing to make the cookies page patterns implemented match our norm text style. However if we delay the use of transport font I think we may need to increase the size of our Helvetica body text anyhow as it appears smaller than Open sans.

phillipgardiner commented 3 years ago

@brockfanning @AnnCorp Regarding the no-javascript content for the cookies page. We had a quick catch up about this in one of our other team meetings. @brockfanning Would you be able to unpick this a bit more and bring along a rough idea of effort and subsequent ramifications of each option to the tech call tomorrow for Ann to make a decision - thanks

brockfanning commented 3 years ago

@LucyGwilliamAdmin This is ready for a refresh for more review.

@phillipgardiner I believe I've addressed all of the items mentioned so far. A few notes:

  1. No privacy policy link yet, because I don't think the page exists (correct me if I'm wrong)
  2. Regarding the no-javascript bit: since the simple approach is simple I went ahead and did that so you can see what I'm talking about (displaying the no-javascript stuff in addition to the normal stuff). I used the notification component.
  3. Regarding the legend for the radio buttons, my thinking is that if it's not necessary for sighted users then it's not necessary for screen-readers either, so I just removed it.
  4. Regarding the cookie for remembering cookie settings: yes we have only one cookie for that. It's called "cookie_settings" right now but that can be changed as needed.
  5. I believe I got all the font-size changes but I suspect there will be more as we notice more things.
brockfanning commented 3 years ago

Discussed in tech call: we'll split the font change into a separate ticket to be done after this one.

brockfanning commented 3 years ago

@LucyGwilliamAdmin This is ready for another refresh, now that the font changes have been postponed to a later ticket.

LucyGwilliamAdmin commented 3 years ago

@brockfanning @phillipgardiner feature branch has been refreshed: http://uk-sdg-feature-branches.s3-website.eu-west-2.amazonaws.com/feature-site-cookies-gds-npm

brockfanning commented 3 years ago

@phillipgardiner Since that feature branches demonstrates the "simple" no-javascript approach (where the no-javascript alert is shown in addition to the regular stuff), I'll also describe the likely approach for a "less simple" no-javascript approach:

  1. Make the footer menu point to "/cookies-no-javascript" instead of "/cookies"
  2. On every page load, use javascript to convert that menu link to "/cookies" instead of "/cookies-no-javascript"

Then the two pages would be maintained separately and could have separate content. I thought I would explain that to show that it's not impossible, just slightly more complicated. Either way is fine with me.

phillipgardiner commented 3 years ago

@brockfanning thanks for making all those changes. I've doubled checked them and they all look great. In regard to the other bits mentioned:

@brockfanning @AnnCorp regarding the no-javascript journey, my preference is for the latter option (e.g. separate page) as its driven by GDS research and also used on ons.gov.uk, and the census websites so keeps the experience consistent. But ultimately i'll leave the call to @AnnCorp. I'm guessing it would be an if statement type thing: If JS = on then display page A, If JS = off, then display page B.

Thanks for doing all this, its looking great.

AnnCorp commented 3 years ago

@brockfanning I agree with @phillipgardiner re approach for no-javascript journey as that is the approach GDS research supports and for UK users to have consistent experience.

LucyGwilliamAdmin commented 3 years ago

@brockfanning if I send doc over slack, can you include privacy policy commit in this PR please?

phillipgardiner commented 3 years ago

@brockfanning @LucyGwilliamAdmin could we please add the 'Privacy' link between 'Cookies' and 'Accessibility statement' in the footer - thanks

brockfanning commented 3 years ago

@LucyGwilliamAdmin This is ready for a refresh.

@phillipgardiner You'll notice that I had to re-word the no-javascript language. Because of the approach we're using (a separate "cookies-no-javascript" page) just "reloading the page" or "turning on javascript" is not enough -- that would just refresh the same no-javascript page. What is needed is for the user to click again on a "Cookies" link.

Also, it is possible distinguish between the different scenarios:

  1. user has javascript disabled
  2. user encountered a javascript glitch

So rather than listing both 1 and 2, I have the no-javascript message only showing the appropriate one.

brockfanning commented 3 years ago

@phillipgardiner Scratch that last comment, I thought of a way to use the original language, by adding a javascript redirect from cookies-no-javascript to cookies.

LucyGwilliamAdmin commented 3 years ago

@brockfanning @phillipgardiner feature branch has been refreshed

phillipgardiner commented 3 years ago

@brockfanning @LucyGwilliamAdmin thanks both

Couple of new bits of feedback spotted:

In regards to the privacy notice

Thanks

LucyGwilliamAdmin commented 3 years ago

@brockfanning @phillipgardiner My bad with the privacy page - it looked fine on my preview but I must not have formatted it correctly. I think possibly an extra line needs to be added in some places? I think the weird indention also might be fixed by this.

phillipgardiner commented 3 years ago

@LucyGwilliamAdmin thats fine, if I can help in anyway let me know :)

brockfanning commented 3 years ago

@LucyGwilliamAdmin The branch is ready for a refresh. Regarding the privacy page, I think maybe some new lines were needed (though I'm not sure why). Let's see if that helps with the issues.

LucyGwilliamAdmin commented 3 years ago

@brockfanning @phillipgardiner feature branch has been refreshed

phillipgardiner commented 3 years ago

@brockfanning @LucyGwilliamAdmin Great, looks good to me :). I've only checked the new changes, not re-checked it all as assuming nothing else changed.

LucyGwilliamAdmin commented 3 years ago

@brockfanning can you resolve the conflicts here when you have a chance please?