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

Move form help text into legend #484

Merged
merged 3 commits into from
Jun 7, 2017
Merged

Move form help text into legend #484

merged 3 commits into from
Jun 7, 2017

Conversation

selfthinker
Copy link
Contributor

@selfthinker selfthinker commented Jun 2, 2017

This is an alternative solution to #485.

What problem does the pull request solve?

Most groups of checkboxes and radio buttons within Elements have an additional introductory help text which isn't a hint. It's usually there to help less technical people understand that you can select multiple checkboxes, but can also be used for other purposes.

This moves that text into the legend to also make it accessible to assistive technologies like screen readers. The styling is nearly the same as before and adds a class to the current paragraph declaration to re-use its styling.

There is a bit more spacing above the help text in the new version (probably 0.26316em which translates to 5px with default settings), even though it uses the same CSS. That is because previously the margins were collapsing as the p came directly after the h3 and now they aren't anymore because the text has been moved into the legend. I'm not sure if it's worth fixing as spacing within Elements is quite inconsistent and it doesn't look wrong.
Let me know if you think I should do something about it.

How has this been tested?

I've tested in multiple browsers via BrowserStack.

Screenshots

Before

form-help-text-before

After

form-help-text-after

Differences

form-help-text-differences

What type of change is it?

  • Bug fix (non-breaking change which fixes an issue)

@gemmaleigh gemmaleigh temporarily deployed to govuk-elements-review-pr-484 June 2, 2017 17:13 Inactive
@gemmaleigh gemmaleigh temporarily deployed to govuk-elements-review-pr-484 June 5, 2017 14:48 Inactive
@gemmaleigh gemmaleigh temporarily deployed to govuk-elements-review-pr-484 June 5, 2017 15:42 Inactive
@gemmaleigh gemmaleigh temporarily deployed to govuk-elements-review-pr-484 June 5, 2017 15:55 Inactive
@selfthinker selfthinker changed the title [Don't merge] Move form help text into legend Move form help text into legend Jun 5, 2017
@@ -20,6 +20,11 @@ fieldset {
width: 100%;
}

// Hack to let legends or elements within legends have margins in webkit browsers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is quite hacky but is the least hacky solution I could find for fixing an issue in all webkits which will not allow a margin on the form hint text. I suspect this might be too controversial (a similar solution was tried in #76 by @timpaul and rejected). That's why I created an alternative solution to fix the same problem with #485.

Copy link

Choose a reason for hiding this comment

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

could use overflow: hidden; instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes that works as well. I could have sworn I tried it but apparently not. I think that makes more sense, so will change it.

@gemmaleigh gemmaleigh temporarily deployed to govuk-elements-review-pr-484 June 6, 2017 12:36 Inactive
Most groups of checkboxes and radio buttons within Elements
have an additional introductory help text which isn't a hint.
It's usually there to help less technical people understand
that you can select multiple checkboxes,
but can also be used for other purposes.

This moves that text into the legend to also make it accessible
to assistive technologies like screen readers.
The styling is nearly the same as before and adds a class
to the current paragraph declaration to re-use its styling.
@selfthinker
Copy link
Contributor Author

I have added some documentation (as suggested by @gemmaleigh on #485), changed the "hack" to use overflow:hidden instead (as suggested by @igloosi) and re-tested in a couple of browsers.

@gemmaleigh
Copy link
Contributor

This looks good, thanks @selfthinker and @igloosi 👍

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.

None yet

3 participants