GOVUK input for filters in around page

[lucascumsille]

Fixes: https://github.com/mysociety/societyworks/issues/3821
Fixes: https://github.com/mysociety/societyworks/issues/4214
Fixes: https://github.com/mysociety/societyworks/issues/4215

IMPORTANT: This update breaks Camden, therefore I think we should tackle this issue before merging this PR. The Alternative I could add a commit here fixing the issue that Camden currently has, which is the line-height property, and leave the tidy up ticket in his original order.

Mobile

Desktop

---

For this commit I replaced multi-select.min.js with the one created on this PR on the jQuery-multi-select repo.

• Clean unnecessary code, specially on _report_list.scss
• Override the default font size at source when compiling the GOVUK CSS so then it would apply everywhere the same(from 19px to 16px)
• Decide best layout horizontal or vertical stacking
• Test on mobile
• Check Camden
• Update labels so we comply with WCAG 2.53 and fix issue 4214

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.61%. Comparing base (ecde96b) to head (dcc4841).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4728      +/-   ##
==========================================
- Coverage   82.61%   82.61%   -0.01%     
==========================================
  Files         395      395              
  Lines       30845    30869      +24     
  Branches     4891     4891              
==========================================
+ Hits        25483    25502      +19     
- Misses       3915     3920       +5     
  Partials     1447     1447              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lucascumsille]

@dracos in order to fix https://github.com/mysociety/societyworks/issues/4214
I was thinking to use aria-labelledby attribute, we would require to add an id for label and the fake label span(Option 1). I would probably make those changes in the plugin itself. However that would only cover the label and .multi-select-button, the creation of the fake label, it's something we do in FixMyStreet(I think), which makes me wonder if we could just undo that, basically not creating the new span and use the label, without hiding it and add aria-label so we can explain better what the select element does(This would be Option 2).

Option 1

 <label class="hidden-js" id="filter_category_label">About - Select a category</label>
  <span id="filter_category_text">About</span>
  
  <span class="multi-select-button" role="button" aria-haspopup="true" tabindex="0" aria-labelledby="filter_category_label filter_category_text">Everything</span>

Option 2

  <label id="filter_category_label" for="filter_category_button">About</label>
  <span class="multi-select-button" id="filter_category_button" role="button" aria-haspopup="true" tabindex="0" aria-labelledby="filter_category_label">Everything</span>

[dracos]

If we're changing the structure to look more like normal fields, it doesn't have to be "About" any more, that's not needed, it's no longer displayed like a readable sentence so doesn't need to be one. It could be "Category". Does that help? Then there's no need for things to be different anywhere. Or have I misunderstood?

[dracos]

(The history of this FMS changing label to span is #3881 and #3863 before that.)

[lucascumsille]

If we're changing the structure to look more like normal fields, it doesn't have to be "About" any more, that's not needed, it's no longer displayed like a readable sentence so doesn't need to be one. It could be "Category". Does that help? Then there's no need for things to be different anywhere. Or have I misunderstood?

To clarify, In this case we would have label + .multi-select-button However in terms of markup should we still include the for attribute to the label for accessibility purpose. I agree with category it should suffice.

[dracos]

The (hidden) label needs to be kept with its for pointing at the hidden <select> for the situation raised by https://github.com/mysociety/societyworks/issues/4214 which is where I changed that. Sorry I missed that bit in the history above.

Stepping back, the TfL issue https://github.com/mysociety/societyworks/issues/4214 as I understood it was the aria-label not matching the label. If the aria-label and the label are both changed to "Category" / "Report status", then they will match? But you're asking could we replace the span with a new label pointing at the button? According to 7bf65b2 and 9ba42de you used a span so that it wouldn't get read out (twice I assume?). So if you want a label now, we can do, but from previous it doesn't sound like that's necessary, as long as the aria-label is updated to match the span.

[lucascumsille]

@dracos I see good to know, I brought the for because I didn't see it in this PR, but I can see it on the live website, so I'll check what is breaking that.
In the mean time I have adjusted the span so now it says "Category"

[lucascumsille]

@dracos for attribute was being removed from the label because of this commit(mysociety/jquery-multi-select@ed762f7) on the jQuery-multi-select plugin. I have added a piece of code here to bring it back.

[lucascumsille]

@dracos Here is a preview of the current state. I have updated the screenshot above as well. When ready I'll rebase and test it on Camden(I'll probably end up getting rid of code that won't be necessary anymore) and all the other cobrands.

[dracos]

for attribute was being removed from the label because of this commit(mysociety/jquery-multi-select@ed762f7) on the jQuery-multi-select plugin. I have added a piece of code here to bring it back.

I was vrery confused, because the for is currently present on the live site, and the live jquery-multi-select includes the commit you're referring to. But then I found my commit 235931e where I altered the FMS jquery-multi-select but didn't put this change in the jquery-multi-select plugin :-/ And when this PR updated the plugin for the new stuff there, that disappeared. So that would explain that! I guess your change is a better way to do it if it has the same effect and doesn't alter the plugin.

[lucascumsille]

I added this commit that should help us with this issue. I added it here because I though the layout of the current site will change with this PR, therefore it made more sense to include it here, but let me know if you think it shouldn't be here.

[dracos]

@lucascumsille I don't think that's what that issue is about - that issue is saying that if you /change/ the filter, it changes automatically without you having to click a Submit, rather than what is happening on page load. The solution is we either add a submit button (meh, guess is easier not to have one, but could do), or add a message saying filters apply automatically (not what /was/ applied).

[lucascumsille]

@dracos I added a fixup for the message commit. At the beginning I thought it would be nice to let the user know what filters have been applied, but yeah it does overcomplicate the ticket.
Let me know if you are happy with this approach.

[dracos]

Looks good, have added fixup for prevAll and to actually have the aria-label on the select match the visible text (which I think was the issue being complained about in 4214). A number of hopefully small issues to deal with

[lucascumsille]

@dracos Thanks for the feedback I have added a fixup for all the issues above with a thumbs up, I think that should be everything. When we are ready I'll tidy up Camden, which most likely will need some amendments with this PR.

[dracos]

This is fine, but you can also have this as one line with:
label.toggleClass('govuk-multi-select__label--checked', this.checked);

[dracos]

Looks good :-) Will need a bit of checking that the changes to report lists/ multi selects etc haven't affected anything else, I guess.

[lucascumsille]

@dracos I found some issues with three cobrands. I couldn't figure out what was causing it, not sure if we are doing something different with them, I couldn't find a template override.

The problem is with "Category" filter, in the markup the select element is not present and also not the multi-select-button

I tried commenting out this line:
$('.js-multiple').make_multi();, but the select element wouldn't be created.

The Status was is fine

• highwaysengland
• isleofwight
• TFL

[lucascumsille]

@dracos I checked all the other cobrands and they are working fine on desktop and mobile.
There were a couple of cobrand that needed some tweaks though.

Here is a preview of them:

[dracos]

Hey, three cobrands - if you haven't set up bodies for them, then it'll just say "Everything" like this:

Is that what you mean? That'll be fine in the real thing if so.

[lucascumsille]

@dracos ohh perfect

Hey, three cobrands - if you haven't set up bodies for them, then it'll just say "Everything" like this:

Then we should be ready.

There is one thing that might be odd, I didn't see it till I went to admin, with this deployment we only have styled the multi-select classes as GOVUK ones, which was fine in the user side of things because I couldn't see an scenario where NON multi-select select elements would be alongside multi-select elements. The admin side of things will have case that look like this:

We currently have this ticket to tackled the admin side which most likely will be next. However I wouldn't mind to add a commit here that changes the style of select elements so they all look like GOVUK ones, no matter which page you are.

[dracos]

I don't think that matters really - that was already the case (if slightly more obvious now). What does the dashboard look like? Currently it's eg

That's the only other place I can think of.

[lucascumsille]

me neither happy to deploy and then tackle all those cases with the rest of tickets. @dracos, let me know if you are happy with this and I'll rebase and merge.
Regarding the commits, I'm thinking of getting rid of the --fixups and squash "Added styling fix for cobrands" into Added GOVUK styling to multiselect plugin.

But let me know if you'd rather have less commits for this PR.

[dracos]

@lucascumsille Doing something about the dashboard as I mentioned in my previous message would be the only thing I'd like to be looked at - it looks a bit odd now:

Can we for now just shrink them there or something like that?

[lucascumsille]

@dracos Sorry I had misread your comment. I added a commit here that reduces the height of the GOVUK ones only for the dashboard. I did't match it exactly, because I would have to reduce the font-size as well, but this approach should get rid of the oddness.

Let me know if you are happy with it and how you'd like me to handle the rebase. I proposed this a few days ago:

Regarding the commits, I'm thinking of getting rid of the --fixups and squash "Added styling fix for cobrands" into Added GOVUK styling to multiselect plugin.

[dracos]

Have squashed it all into one commit - let me know what you think!

[lucascumsille]

Thanks @dracos looks good to me, thanks for adding it to the changelog as well =)

[dracos]

Not sure how I didn't notice this before, sorry, but this should presumably be a legend not a fieldset.

[lucascumsille]

Thanks @dracos well spotted. I create a PR here:
#5069