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

Ability to group radio buttons / checkboxes within sub-headings #1266

Closed

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Apr 1, 2019

This feature adds a new macro govukFormGroup().

This lets us put radios/checkboxes inside a <div class="govuk-form-group"> wrapper, nested to multiple levels with the describedBy variable to be persisted and appended to at each level.

It also allows the component examples to show both regular and fieldset-wrapped variations using the optional params.fieldset, useful to confirm bugs like #1264

Form group

Why else is it needed?

I've also addressed what looks like a bit of technical debt.

Duplicated inside almost all form control macros is Nunjucks code to generate a govukLabel() label, a govukHint() hint an a govukErrorMessage() error message.

This code here is pretty much copy and pasted 7 times:

  {{ govukLabel({
    html: params.label.html,
    text: params.label.text,
    classes: params.label.classes,
    isPageHeading: params.label.isPageHeading,
    attributes: params.label.attributes,
    for: params.id
  }) | indent(2) | trim }}
{% if params.hint %}
  {% set hintId = params.id + '-hint' %}
  {% set describedBy = describedBy + ' ' + hintId if describedBy else hintId %}
  {{ govukHint({
    id: hintId,
    classes: params.hint.classes,
    attributes: params.hint.attributes,
    html: params.hint.html,
    text: params.hint.text
  }) | indent(2) | trim }}
{% endif %}
{% if params.errorMessage %}
  {% set errorId = params.id + '-error' %}
  {% set describedBy = describedBy + ' ' + errorId if describedBy else errorId %}
  {{ govukErrorMessage({
    id: errorId,
    classes: params.errorMessage.classes,
    attributes: params.errorMessage.attributes,
    html: params.errorMessage.html,
    text: params.errorMessage.text,
    visuallyHiddenText: params.errorMessage.visuallyHiddenText
  }) | indent(2) | trim }}
{% endif %}

It's used by textarea, select, radios, checkboxes, text input, file upload and date input.

All this boilerplate code is now handled by govukFormGroup().

@colinrotherham
Copy link
Contributor Author

Can we add a Heroku deployment please?

I've added a new radios/checkboxes with subheadings demo to the form examples page.

Thanks

@colinrotherham colinrotherham force-pushed the checkbox-radio-subheadings branch 2 times, most recently from 0b483a9 to aeb01c0 Compare April 2, 2019 08:12
@colinrotherham colinrotherham marked this pull request as ready for review April 2, 2019 08:29
@colinrotherham colinrotherham force-pushed the checkbox-radio-subheadings branch from aeb01c0 to 1c73bc6 Compare April 2, 2019 09:00
@colinrotherham
Copy link
Contributor Author

Pushing again, looks like I got auto-cancelled:

This job was cancelled because the "Auto Cancellation" feature is currently enabled, and a more recent build (#7798) for pull request #1266 came in while this job was waiting to be processed.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1266 April 2, 2019 11:15 Inactive
@colinrotherham
Copy link
Contributor Author

colinrotherham commented Apr 2, 2019

Thanks for the deployment @hannalaakso.

@frankieroberto I've added the radios/checkboxes within subheadings examples here:
https://govuk-frontend-review-pr-1266.herokuapp.com/examples/form-elements

@colinrotherham colinrotherham force-pushed the checkbox-radio-subheadings branch from 1c73bc6 to 3012dfb Compare April 2, 2019 15:14
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1266 April 2, 2019 15:15 Inactive
@dashouse dashouse added the awaiting triage Needs triaging by team label Apr 3, 2019
@colinrotherham
Copy link
Contributor Author

As mentioned by @frankieroberto, I'll still need an alternative to #1163 otherwise open conditional reveals won't close when another group's radio is clicked.

I'll take a look at this today.

@colinrotherham colinrotherham force-pushed the checkbox-radio-subheadings branch from 3012dfb to 4377115 Compare April 3, 2019 13:06
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1266 April 3, 2019 13:06 Inactive
@colinrotherham
Copy link
Contributor Author

colinrotherham commented Apr 3, 2019

I've just added an alternative fix to #1163.

This adds support for "Conditional reveal across sibling form groups"

Sibling-form-group-conditional-reveal

Looks like the final piece required for #1079 (including #1166 solved via govukFormGroup())

It scopes radio events a little higher so we support:

The default (as it works now)

.govuk-fieldset input[type="radio"]

Form group

.govuk-form-group input[type="radio"]

Form group with nested form groups
E.g. The radio buttons with sub-headings pattern from this ticket

.govuk-form-group .govuk-form-group input[type="radio"]

@colinrotherham colinrotherham force-pushed the checkbox-radio-subheadings branch from 4377115 to c5c0e29 Compare April 3, 2019 13:16
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1266 April 3, 2019 13:16 Inactive
@kellylee-gds kellylee-gds added Effort: days ⚠️ high priority Used by the team when triaging and removed awaiting triage Needs triaging by team labels Apr 3, 2019
@colinrotherham
Copy link
Contributor Author

colinrotherham commented Apr 3, 2019

Initial testing in IE8–11 for the "Conditional reveal across sibling form groups" feature works great

@colinrotherham colinrotherham force-pushed the checkbox-radio-subheadings branch from c5c0e29 to 244bf2e Compare April 3, 2019 17:33
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1266 April 3, 2019 17:34 Inactive
@colinrotherham colinrotherham force-pushed the checkbox-radio-subheadings branch from 244bf2e to 4b152c4 Compare April 4, 2019 07:59
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1266 April 4, 2019 07:59 Inactive
@colinrotherham
Copy link
Contributor Author

Just added '../../vendor/polyfills/Element/prototype/closest' to radios.js so we've covered if anyone imports the file separately.

@aliuk2012
Copy link
Contributor

aliuk2012 commented Apr 5, 2019

Hi @colinrotherham

Firstly, we would like to thank you for submitting this PR and for the tremendous amount of work that you have put into this PR.
We have discussed this change as a team and would like to take this to the next GOVUK Design System Working Group on 25 April to get assurance that the pattern works from a usability point of view.

Until a decision is made we won't be requiring anything more from you and we'll keep you updated as to the progress.

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Apr 5, 2019

Thanks @aliuk2012.

I appreciate there are three distinct pieces of work you might want me to separate out:

  1. Removing duplication of label/hint/error with govukFormGroup() macro
  2. Allowing params.fieldset.describedBy to be configured (to pass to nested fieldsets)
  3. Supporting conditional reveal across sibling radio lists

We can do all or any of these in isolation.

See what you think at the working group.

@colinrotherham
Copy link
Contributor Author

Rebased with master

@aliuk2012
Copy link
Contributor

Hi @colinrotherham,

Just a quick update, we were reviewing what we were going to take to the working group this week and we have decided not to take this pattern to the working group. The reason is we do not feel like this would be a good pattern to introduce and there are a lot of usability issues with it.

Regarding the other changes you have proposed in this PR, we still think that they might still be worth merging but we need to discuss this as a team.

Until a decision is made we won't be requiring anything more from you and we'll keep you updated as to the progress.

@colinrotherham colinrotherham force-pushed the checkbox-radio-subheadings branch from 87f97b3 to 5f2a8eb Compare April 23, 2019 12:48
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1266 April 23, 2019 12:48 Inactive
@colinrotherham
Copy link
Contributor Author

@aliuk2012 Ah no problem.

I appreciate this pattern confuses the One thing per page approach, and it's more applicable to our staff-facing systems so we can replicate it there instead.

The other tidy-up changes

I've removed the new pattern commits, and pushed the other changes here: #1281

@colinrotherham colinrotherham force-pushed the checkbox-radio-subheadings branch from 5f2a8eb to 370b427 Compare April 23, 2019 13:19
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1266 April 23, 2019 13:19 Inactive
@colinrotherham
Copy link
Contributor Author

Rebased with form-group-wrapper from #1281 so these are now separate.

@aliuk2012
Copy link
Contributor

We are closing this PR as it was decided that this pattern had a lot of usability issues but we are still considering the other work that @colinrotherham has done but the additional work has now been split out into two separate PR (#1281, #1297)

@aliuk2012 aliuk2012 closed this Apr 25, 2019
@aliuk2012 aliuk2012 removed the blocked label Apr 25, 2019
@colinrotherham colinrotherham deleted the checkbox-radio-subheadings branch May 16, 2019 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ high priority Used by the team when triaging
Projects
Development

Successfully merging this pull request may close these issues.

5 participants