-
Notifications
You must be signed in to change notification settings - Fork 334
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
Make Character Count use hint component for message and allow custom … #1650
Make Character Count use hint component for message and allow custom … #1650
Conversation
The changelog entry in this pull request will need updating as v3.4.0 has now been released. |
@36degrees Thank you - that's updated now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, the new API option countMessageClasses
aligns with the BEM class...
There's a few minor changes needed before we can get this in, if you don't have time let us know and we can try to finish this up when we do. :)
CHANGELOG.md
Outdated
|
||
You can now add custom classes to the count message in the character count component, which has been changed to use the hint component rather than custom markup. | ||
|
||
- [Pull request #:1650: Make Character Count use hint component for message and allow custom classes to be added](https://github.com/alphagov/govuk-frontend/pull/1650) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny style thing: "Pull request #:1650" -> "Pull request: #1650"
CHANGELOG.md
Outdated
|
||
#### Add custom classes to the count message in the character count component | ||
|
||
You can now add custom classes to the count message in the character count component, which has been changed to use the hint component rather than custom markup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When writing CHANGELOG notes we try to focus on what the user needs to know, so how we fixed or updating something is not required to understand the new feature.
So this could instead focus on how to use the new countMessageClasses
option rather than the internal details on how we implemented this change.
<span id="{{ params.id }}-info" class="govuk-hint govuk-character-count__message" aria-live="polite"> | ||
You can enter up to {{ params.maxlength or params.maxwords }} {{'words' if params.maxwords else 'characters' }} | ||
</span> | ||
{{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny style thing: We keep the macro on the same line as the curly
{{ govukHint({
...
}) }}
vs
{{
govukHint({
...
})
}}
@@ -141,3 +145,12 @@ examples: | |||
threshold: 75 | |||
label: | |||
text: Full address | |||
|
|||
- name: with count message class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where possible we try to add tests rather than review application demonstrations for non-visible functionality. So we remove this example and test this with Jest rather than having to check the review application.
b16caec
to
a7c2346
Compare
@@ -59,6 +59,10 @@ params: | |||
type: object | |||
required: false | |||
description: HTML attributes (for example data attributes) to add to the textarea. | |||
- name: countMessageClasses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For other reviewers, should this be countMessage
or message
with a nested classes
option?
Could allow for updates without breaking changes in the future.
{{ govukCharacterCount({
label: {
text: "Can you provide more detail?"
},
countMessageClasses: 'app-extra-class'
}) }}
vs
{{ govukCharacterCount({
label: {
text: "Can you provide more detail?"
},
countMessage: {
classes: 'app-extra-class'
}
}) }}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd tend towards the nested object, as it's likely we'll need to make the text customisable in the future (somehow) so that it can be overridden if you're using a different language.
hint: { | ||
text: 'It’s on your National Insurance card, benefit letter, payslip or P60. For example, ‘QQ 12 34 56 C’.' | ||
} | ||
}, | ||
countMessageClasses: 'app-extra-class' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should probably be tested in a separate assertion – this test is looking at whether a separate actual 'hint' can be added to the character count ("'It’s on your National Insurance card, benefit letter, payslip or P60. For example, ‘QQ 12 34 56 C’.'") whereas this class is added to the count message, which is unrelated to the rest of the test. I'd also assert that the class is on the element, rather than using a snapshot for it.
id: params.id + '-info', | ||
classes: 'govuk-character-count__message' + (' ' + params.countMessageClasses if params.countMessageClasses), | ||
attributes: { | ||
'aria-live': 'polite' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this isn't 'new', but we're changing the implementation and it doesn't look like there's a test checking that this attribute is set correctly. Given how important this is for assistive tech, could we add one?
@@ -59,6 +59,10 @@ params: | |||
type: object | |||
required: false | |||
description: HTML attributes (for example data attributes) to add to the textarea. | |||
- name: countMessageClasses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd tend towards the nested object, as it's likely we'll need to make the text customisable in the future (somehow) so that it can be overridden if you're using a different language.
<span id="{{ params.id }}-info" class="govuk-hint govuk-character-count__message" aria-live="polite"> | ||
You can enter up to {{ params.maxlength or params.maxwords }} {{'words' if params.maxwords else 'characters' }} | ||
</span> | ||
{{ govukHint({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still a little unsure about this change – it generally feels a bit strange to me that we're using the hint component at all here (as opposed to just having a 'character count message' which happens to look a bit like a hint…) – however, I don't think this makes it any harder for us to change this later, so I think it's OK for now.
Update the tests so there is coverage for the count message functionality and include new functionality.
a7c2346
to
6abc78b
Compare
@36degrees Updated the tests so there is coverage for the count message functionality and include new functionality. |
…classes to be added
Character count was using the hint markup but not the component for its count message. This PR changes that to use the hint component. It doesn't allow custom text or id as these are referred to in the javascript, but it does allow custom classes to be added for styling.
Relevant issue: #1645