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

Allow form group classes on radios and checkboxes #1043

Merged
merged 3 commits into from
Oct 31, 2018

Conversation

kr8n3r
Copy link

@kr8n3r kr8n3r commented Oct 25, 2018

What

Make it possible to toggle the govuk-form-group--error class on the form group for radios and checkboxes without having to pass an error message.

Why

Currently the error state for a conditionally revealed checkbox shows the 'error bar' against the revealed content only.
68747470733a2f2f7472656c6c6f2d6174746163686d656e74732e73332e616d617a6f6e6177732e636f6d2f3539616436653537653761636164313862643863623437362f3562613463663131396438346530343934353038353766352f6437336264323764623664363464363035626634643330316631

We want to move the error bar to the 'question' which will include the revealing checkboxes or radios themselves.

To do this we have in introduced a `formGroupClasses' option in the Nunjucks macro that allows a CSS class to be set directly on the form group.

Example of an error state set on the whole group

screen shot 2018-10-25 at 09 52 31

screen shot 2018-10-25 at 09 52 47

Trello ticket: https://trello.com/c/LggtR8s5/1455-make-it-possible-to-set-the-error-state-on-checkboxes-and-radios-without-passing-an-error-message

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1043 October 25, 2018 08:48 Inactive
@kr8n3r kr8n3r force-pushed the allow-form-group-classes-on-radios-and-checkboxes branch from 845be65 to f91a830 Compare October 25, 2018 08:53
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1043 October 25, 2018 08:54 Inactive
@NickColley
Copy link
Contributor

I think if we want to add this potentially we want to make sure you can set formGroupClasses anywhere that formGroups are used? Or are these the only places?

Just thinking people will expect this to be consistent across components.

@kr8n3r
Copy link
Author

kr8n3r commented Oct 30, 2018

@NickColley most likely, see #1042
scoping work for to the trello ticket. if we agree on the format of the parameter (this or one proposed in the linked issue) and we can roll that out to other components that group elements

@36degrees
Copy link
Contributor

Are there other things that we can foresee users wanting to add form-groups in the future, like attributes? Just wondering if we should opt for formGroup: { classes: '' } instead.

I also wonder whether this is one area where providing a boolean flag (e.g. error: true) might be helpful (we may want both).

@kr8n3r
Copy link
Author

kr8n3r commented Oct 30, 2018

formGroup: { classes: '' } is more flexible, though more verbose. Not sure what else whey'd want to add to the form group, but i'm probably mistaken.

would error :true additional option be more confusing? what if you want both classes and the error state?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1043 October 30, 2018 13:33 Inactive
@kr8n3r kr8n3r force-pushed the allow-form-group-classes-on-radios-and-checkboxes branch from f91a830 to 2080e92 Compare October 30, 2018 14:25
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1043 October 30, 2018 14:25 Inactive
@kr8n3r kr8n3r force-pushed the allow-form-group-classes-on-radios-and-checkboxes branch from 2080e92 to 5bbfbed Compare October 30, 2018 14:26
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1043 October 30, 2018 14:26 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1043 October 30, 2018 14:29 Inactive
igloosi added 3 commits October 30, 2018 14:30
…error message

Currently the error state for conditionally revealed content
shows the 'error bar' against the revealed content only.

We want to move the error bar to the 'question' which will include the revealing radios themselves.
… an error message

Currently the error state for a conditionally revealed content shows the 'error bar' against the revealed content only.

We want to move the error bar to the 'question' which will include the revealing checkboxes themselves.
@kr8n3r
Copy link
Author

kr8n3r commented Oct 30, 2018

rewritten to accept formGroup: { classes: '' } way of passing options to the macro

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@kr8n3r kr8n3r added this to the [NEXT] milestone Oct 31, 2018
@kr8n3r kr8n3r merged commit eeb1b16 into master Oct 31, 2018
@kr8n3r kr8n3r deleted the allow-form-group-classes-on-radios-and-checkboxes branch October 31, 2018 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants