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

Add select component hint #2594

Merged
merged 1 commit into from
Jan 31, 2022
Merged

Add select component hint #2594

merged 1 commit into from
Jan 31, 2022

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented Jan 28, 2022

What

Adds a option for a hint to the select component.

  • based on the Design system
  • an optional hint attribute, with optional hint id
  • the aria-describedby attribute of the select points to the hint, if an error is also present the describedby attribute includes both hint and error IDs

During this work I noticed that there's a lot of code in the template and some other refactoring that should be looked at (e.g. the helper file isn't named correctly - it's select.rb instead of select_helper.rb) but I think I'll do that separately to not over-complicate this PR.

Why

Needed in smart-answers.

Visual Changes

Screenshot 2022-01-28 at 15 06 59

Trello card: https://trello.com/c/P0YMGgDs/594-add-hint-to-select-component

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2594 January 28, 2022 15:21 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2594 January 28, 2022 15:26 Inactive
@@ -37,6 +48,10 @@
<%= label_tag(id, label, class: label_classes) %>
<% end %>

<% if hint_text %>
<%= tag.span hint_text, id: hint_id, class: "govuk-hint" %>
Copy link
Contributor

Choose a reason for hiding this comment

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

just to note that in govuk-frontend v3.8 span became div and in v4 styles which made the hint display as block were removed

migration can either be addressed here or in #2533

Copy link
Contributor

Choose a reason for hiding this comment

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

The hint component already uses a div element. However, we have two instances (character count and checkboxes) where the component is not being used and a span is still in place. I have updated #2533 to address these two special cases (until we update them to use the hint component). For this PR I suggest we use the hint component as we do for most form components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the input both. Yes, agree that we should definitely use the hint component here, have changed 🤦

Copy link
Contributor

@kr8n3r kr8n3r Jan 31, 2022

Choose a reason for hiding this comment

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

The hint component already uses a div element. However, we have two instances (character count and checkboxes) where the component is not being used and a span is still in place. I have updated #2533 to address these two special cases (until we update them to use the hint component). For this PR I suggest we use the hint component as we do for most form components.

sorry didn't see you folk have a hint component 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the nudge, @kr8n3r – otherwise I wouldn't have found the two exceptions 😄

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2594 January 31, 2022 14:21 Inactive
Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Once comment related to the hint variable name we use in the component API, otherwise good to be merged.

@@ -10,12 +10,23 @@
heading_size = false unless shared_helper.valid_heading_size?(heading_size)
error_message ||= nil
error_id ||= nil
hint_text ||= nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I seems like we use hint for the hint text, looking at other component's APIs. I would stick to hint for consistency, although I appreciate hint_text might be clearer.

- based on the Design system
- an optional hint attribute, with optional hint id
- the aria-describedby attribute of the select points to the hint, if an error is also present the describedby attribute includes both hint and error IDs
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2594 January 31, 2022 15:35 Inactive
@andysellick andysellick merged commit 2bf8bae into master Jan 31, 2022
@andysellick andysellick deleted the add-select-hint branch January 31, 2022 16:48
@jon-kirwan jon-kirwan mentioned this pull request Feb 1, 2022
KyleMacPherson added a commit to KyleMacPherson/govuk_publishing_components that referenced this pull request Feb 4, 2022
Prior to this change, the documentation for the select component suggests passing a parameter called `hint_text`,
however it looks that this should actually be `hint`. Comment on hint_text vs hint on original PR:
alphagov#2594 (comment)
@KyleMacPherson KyleMacPherson mentioned this pull request Feb 4, 2022
alex-ju pushed a commit to KyleMacPherson/govuk_publishing_components that referenced this pull request Feb 4, 2022
Prior to this change, the documentation for the select component suggests passing a parameter called `hint_text`, however it looks that this should actually be `hint`. Comment on hint_text vs hint on original PR:
alphagov#2594 (comment)
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