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 character count component #959

Merged
merged 25 commits into from Oct 11, 2018

Conversation

@alex-ju
Copy link
Member

commented Aug 17, 2018

This PR is a proposal for integrating the character count component to govuk-frontend.

Preserved optional features

Threshold value

  • defined via data-threshold="75" (recommended) or .init( { threshold: 75 } )
  • if unset defaults to 0 — the message is displayed from the beginning
  • if set to a certain value (e.g. 75) the message is displayed after 75% of the maximum limit is reached

Word count

  • defined via data-maxwords="200" (recommended) or .init( { maxwords: 200 } )
  • if unset defaults to counting characters
  • if set counts words instead of characters

Hint and Error message

  • using the textarea component

Dropped optional features

  • highlight option (needs more user research and limits the resize of the textarea)
  • validation border option (the validation border is part of the Error message component and is applied to the Character count component if an error occurs after the page is submitted)

Known issues

JAWS repeats the label of the textarea each time an aria-live associated with the textarea is updated.

Design System backlog
Trello card

@alex-ju alex-ju changed the title Add character count component [DNM] Add character count component Aug 17, 2018

@hannalaakso
Copy link
Member

left a comment

Thanks a lot for this @alex-ju, it's looking great 👍 I left a few comments but I didn't get through all of it, will continue reviewing this next week.

if (this.maxLength && this.countMessage) {
// Replace maxlength attribute with data-maxlength to avoid hard limit
$module.setAttribute('maxlength', '')
$module.setAttribute('data-maxlength', $module.maxLength)

This comment has been minimized.

Copy link
@hannalaakso

hannalaakso Aug 17, 2018

Member

Should these be done on the textarea instead? Thedata-maxlength on the outer wrapper is shown as undefined.

This comment has been minimized.

Copy link
@alex-ju

alex-ju Aug 20, 2018

Author Member

I'm skeptic about moving these to the textarea instead because in my opinion they are being used at the component level not at the textarea element level. Also we should keep it flexible and easier to extend in the future so that the field could be an input or a textarea.

This comment has been minimized.

Copy link
@hannalaakso

hannalaakso Aug 20, 2018

Member

My bad I assumed maxlength was set on $textarea. The maxlength value should be from the character count instance, not from the module as $module.maxLength is undefined. Think it should be:

$module.setAttribute('data-maxlength', this.maxLength)
var length
if (options.maxwords) {
// var tokens = text.split(' ')
// length = tokens.length-1

This comment has been minimized.

Copy link
@hannalaakso

hannalaakso Aug 17, 2018

Member

Could these comments be deleted?

This comment has been minimized.

Copy link
@alex-ju

alex-ju Aug 20, 2018

Author Member

Yep, removed them at cleanup.

if (remainingNumber < 0) {
countElement.classList.add('govuk-textarea--error')
if (options.validation) {
countElement.parentNode.parentNode.classList.add('govuk-form-group--error')

This comment has been minimized.

Copy link
@hannalaakso

hannalaakso Aug 17, 2018

Member

I tested this locally be setting validation: true and the form group error class gets set on the wrong div so that just needs changing. Would it be possible to do this more robustly by looking for the form-group class and adding the class to that?

This comment has been minimized.

Copy link
@alex-ju

alex-ju Aug 20, 2018

Author Member

Validation border via JavaScript is no longer supported as we'll only use it for server side validation. Removed the code and updated the PR description to reflect that.

src/components/character-count/character-count.js Outdated Show resolved Hide resolved
text: "Can you provide more detail?",
hintText: "Don't include personal or financial information, eg your National Insurance number or credit card details."
}
}) }}

This comment has been minimized.

Copy link
@hannalaakso

hannalaakso Aug 20, 2018

Member

I don't think we have this file for other components. Does it help to demonstrate how you would use the component?

This comment has been minimized.

Copy link
@36degrees

36degrees Aug 20, 2018

Contributor

Example files were removed in #890 – I imagine the work on this started before that change was made.

This comment has been minimized.

Copy link
@alex-ju

alex-ju Aug 21, 2018

Author Member

Wasn't aware. Will check and update.


CharacterCount.prototype.handleFocus = function () {
// Check if value changed on focus
this.valueChecker = setInterval(this.checkIfValueChanged.bind(this), 1000)

This comment has been minimized.

Copy link
@nickcolley

nickcolley Aug 21, 2018

Contributor

I wonder if we can take advantage of this possible alternative:

alphagov/accessible-autocomplete#164 (comment)

This comment has been minimized.

Copy link
@alex-ju

alex-ju Aug 21, 2018

Author Member

Good suggestion. I'll try.

This comment has been minimized.

Copy link
@nickcolley

nickcolley Aug 21, 2018

Contributor

Hmm, does not support IE8. But we could be OK with this since it'll likely only be used for users of assistive technologies that we can assume will be using a newer version of IE?

This comment has been minimized.

Copy link
@nickcolley

nickcolley Oct 5, 2018

Contributor

Non-blocking: Did you try this out, we could raise an issue later to investigate?

This comment has been minimized.

Copy link
@alex-ju

alex-ju Oct 9, 2018

Author Member

Tried it and behaves as expected (starts on focus, stops on blur). Could do a second test to be sure.

This comment has been minimized.

Copy link
@nickcolley

nickcolley Oct 10, 2018

Contributor

I'll raise an issue after we merge this to investigate the approach defined in that issue.


CharacterCount.prototype.handleFocus = function () {
// Check if value changed on focus
this.valueChecker = setInterval(this.checkIfValueChanged.bind(this), 1000)

This comment has been minimized.

Copy link
@nickcolley

nickcolley Aug 21, 2018

Contributor

Hmm, does not support IE8. But we could be OK with this since it'll likely only be used for users of assistive technologies that we can assume will be using a newer version of IE?

@fofr

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

A side note on this feature, might not be relevant to this PR directly: I think the logic (eg the regex) that determines how many words are present should be surfaced somehow, as this will need to be reused in any server side validation so that both frontend and backend are consistent.

@alex-ju

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2018

Updated the component code based on the working group feedback and rebased.
I think this is now good to be reviewed by the Design System team for integration.

@alex-ju alex-ju changed the title [DNM] Add character count component Add character count component Oct 2, 2018

var countAttribute = (this.options.maxwords) ? this.defaults.wordCountAttribute : this.defaults.characterCountAttribute

// Set the element limit
if ($module.getAttribute) {

This comment has been minimized.

Copy link
@nickcolley

nickcolley Oct 5, 2018

Contributor

Can you explain this if statement? I'm not sure this would ever fail?

This comment has been minimized.

Copy link
@alex-ju

alex-ju Oct 5, 2018

Author Member

Haha.. good catch. It was $module.getAttribute(countAttribute) to make sure an attribute is set and if not the script will stop, but got lost on it's way.. will update!

// If there's a maximum length defined and the count message was successfully applied
if (this.maxLength && this.countMessage) {
// Replace maxlength attribute with data-maxlength to avoid hard limit
$module.setAttribute('maxlength', '')

This comment has been minimized.

Copy link
@nickcolley

nickcolley Oct 5, 2018

Contributor

Do we have a maxlength to replace anymore?

This comment has been minimized.

Copy link
@joelanman

joelanman Oct 5, 2018

Member

if this is what I think it is, it's relevant to something I was concerned about - if users don't use the macro they could well add maxlength to the texarea html tag themselves. We could remove it in code as it looks like we are doing here, and/or talk about this aspect in the guidance ('do not add 'maxlength' because ...)

This comment has been minimized.

Copy link
@alex-ju

alex-ju Oct 5, 2018

Author Member

Yes, just spoke about this with @joelanman. This is removing the hard limit if any set (needs a bit of refactor and I'll use removeAttribute instead of setting an empty string). How do you feel about this approach @nickcolley?

This comment has been minimized.

Copy link
@nickcolley

nickcolley Oct 5, 2018

Contributor

Feels reasonable if we think this would be an accident for users to do this, would be good to add a comment to explain this.


CharacterCount.prototype.handleFocus = function () {
// Check if value changed on focus
this.valueChecker = setInterval(this.checkIfValueChanged.bind(this), 1000)

This comment has been minimized.

Copy link
@nickcolley

nickcolley Oct 5, 2018

Contributor

Non-blocking: Did you try this out, we could raise an issue later to investigate?

{# {% set componentGuidanceLink = 'new link here' %} #}

{% block componentArguments %}
{{ govukTable({

This comment has been minimized.

Copy link
@nickcolley

nickcolley Oct 5, 2018

Contributor

We have moved component arguments into the .yaml file now, we can do this for you...

@nickcolley

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2018

Our components tend to have a consistent margin at the bottom, I wonder if we need one here too?

Regular text area:
screen shot 2018-10-05 at 12 26 15

Character count:
screen shot 2018-10-05 at 12 26 24

{% set describedBy = "" %}

<div class="govuk-character-count" data-module="character-count"
{% if params.maxlength %} data-maxlength="{{ params.maxlength }}"{% endif %}

This comment has been minimized.

Copy link
@nickcolley

nickcolley Oct 5, 2018

Contributor

Non-blocking, if we used the whitespace stripping syntax these would end up on the same line when the markup is rendered.

<div class="govuk-character-count" data-module="character-count"
data-maxlength="blah"
></div>

vs

<div class="govuk-character-count" data-module="character-count" data-maxlength="blah"></div>
@alex-ju

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2018

@nickcolley thank you for reviewing this! update this PR with 4 commits addressing your comments.

@hannalaakso
Copy link
Member

left a comment

Fab work @alex-ju. Just need a CHANGELOG entry, the new README committed and another person from the team to approve this 👏

alex-ju and others added some commits Jun 27, 2018

Integrate govuk-texarea markup to character count
This commit:
- removes the `govuk-character-count` markup and includes the textarea
Nunjucks macro instead
- add the label and hint with the texarea
- removes now redundant "govuk-form-group" wrapper - this is now
added with textarea macro
- adds an outer wrapper div with `data-module=character-count`,
this div also now has maxlength/maxwords set on it and renders classes

Note that the hint work is not part of this card - for now, the hint
is being passed through to the textarea to get the aria-describedby
to show up.
Adjust CSS to use govuk-textarea with character count
This commit:

- removes the `govuk-character-count` CSS as  we now use the
textarea styling for this (but leaves some classes to do with errors
as these will be tackled in the hint work)
-  however, this does add CSS to adjust padding to stop the
textarea from "jumping" when the border width changes (on adding
error class)
Adjust JS to use govuk-textarea with character count
Ajust JS so that $module is now `.js-character-count` inside
the component and that maxlength/maxwords is grabbed from parent
component.
Fix tests for character count with govuk-textarea
Fix tests by using `js-character-count` for tests as this
contains the functionality - although this
is different from how we do it in other components, this should
be looked at as part of the tests card.

The hint and error message tests are still failing as they
are "hijacked" by the character count - we'll look at this
as part of the hint card.
Allow hint and error messages to be passed to textarea
and rely on JavaScript for the info message
@alex-ju

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2018

Rebased against master again 🤞

// Check for existing info count message
var countMessage = document.getElementById(elementId + '-info')
// If there is no existing info count message we add one right after the field
if (elementId && !countMessage) {

This comment has been minimized.

Copy link
@nickcolley

nickcolley Oct 10, 2018

Contributor

I think we no longer need to create this count message, since we have a progressively enhanced way of doing this.

I don't think this is blocking but thought it was worth mentioning since it could simplify a lot of logic.

This comment has been minimized.

Copy link
@alex-ju

alex-ju Oct 10, 2018

Author Member

You're right about this, we can now remove the createCountMessage function and the associated logic; what I'm questioning thought is what will happen if they forget to add the default message. With the macro we can ensure that, but without it may get lost. I'm up for simplifying the code if we think people will add the default message.

This comment has been minimized.

Copy link
@nickcolley

nickcolley Oct 11, 2018

Contributor

Yeah I suppose this would force people to do a more progressive enhancement approach, but even as is I think it's still good to go.

var thresholdPercent = options.threshold ? options.threshold : 0
var thresholdValue = maxLength * thresholdPercent / 100
if (thresholdValue > currentLength) {
countMessage.classList.add('govuk-character-count__message--disabled')

This comment has been minimized.

Copy link
@nickcolley

nickcolley Oct 10, 2018

Contributor

Does this need to be polyfilled? (Maybe related to #674)

This comment has been minimized.

Copy link
@alex-ju

alex-ju Oct 10, 2018

Author Member

Added the classList polyfill


CharacterCount.prototype.handleFocus = function () {
// Check if value changed on focus
this.valueChecker = setInterval(this.checkIfValueChanged.bind(this), 1000)

This comment has been minimized.

Copy link
@nickcolley

nickcolley Oct 10, 2018

Contributor

I'll raise an issue after we merge this to investigate the approach defined in that issue.


{#- a record of other elements that we need to associate with the input using
aria-describedby – for example hints or error messages -#}
{% set describedBy = "" %}

This comment has been minimized.

Copy link
@nickcolley

nickcolley Oct 10, 2018

Contributor

I'm not following how describedBy gets set to something other than '', where does that happen?

This comment has been minimized.

Copy link
@alex-ju

alex-ju Oct 10, 2018

Author Member

I added the id of the default message as a default value for describedBy - which can be amended with hint and error message ids in the textarea component.

@nickcolley

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2018

@alex-ju I've added some more review comments, I think the only one that is blocking is the polyfill one if we do need to polyfill classList

@nickcolley

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2018

This is excellent work, thanks so much Alex this will helps lots of teams across GOV. I love the care that has gone into the research and efforts to make this component work well in assistive technologies like Dragon. 👏

If we can release this today we might be able to publish it today 🚀 !

@nickcolley nickcolley merged commit f9e473e into alphagov:master Oct 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kr8n3r kr8n3r added this to the [NEXT] milestone Oct 11, 2018

@alex-ju alex-ju deleted the alex-ju:add-character-count-component branch Oct 11, 2018

@kr8n3r kr8n3r referenced this pull request Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.