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

Refactor handling of count message in character count JS #1594

Merged
merged 4 commits into from
Oct 7, 2019

Conversation

hannalaakso
Copy link
Member

@hannalaakso hannalaakso commented Sep 26, 2019

The “count message” refers to the “You have x characters remaining” message in the character count (govuk-character-count__message).

Original issue

The original issue #1106 raised by @36degrees was fixed by #1347. This allowed a value for aria-describedby to be passed through to the textarea component. This means that the character count component can correctly pass the id of the count message to the textarea component to associate the two.

Fixes in this PR

There was a bit of refactoring to do in terms of how the JS of the character count handles defining the count message so this PR does that. I’ve tried to break the changes into smaller commits to make it clear what’s going on but can squash them before merging if that makes more sense.

Making the count message markup required

The count message markup is now always present (since 7b534bf). This PR makes the markup for it required, instead of giving the option to add it with Javascript - the message markup should always exist so that user is notified of the character limit, even if JS doesn’t run.

This PR removes the branching logic and the code that added the count message markup dynamically in createCountMessage(). It’s worth nothing that the code inside the block hasn’t been executed since 7b534bf. As using the character count without the count message markup and relying it to be added with JS is not part of the public API, removing this functionality is not considered a breaking change.

Defining the count message

The count message DOM element is now defined at the top of the module with the other required DOM elements.

We check whether the textarea is present before getting its id to get the count message DOM element - the check prevents an error with getting $textarea.id if the script is run without component markup being present present on the page. Alternatively we could define the count message in init() after doing a check for textarea which would require another check for it and wouldn’t make it as clear that it’s required as when placed at the top of module. See below for the code. Thoughts?

CharacterCount.prototype.init = function () {
  var $module = this.$module
  var $textarea = this.$textarea

  if (!$textarea) {
    return
  }

  this.$countMessage = $module.querySelector('[id=' + this.$textarea.id + '-info]')

  if (!$countMessage) {
    return
  }

Moving the count message markup inside govuk-form-group

The original code for checking/creating the count message markup moves it in the DOM to be inside govuk-form-group. This was probably useful when the message markup was created dynamically. This PR retains the bit of code that moves the element for backwards compatibility. It’s important that the count message markup is inside govuk-form-group so that it gets wrapped within the error block (see screen grab below).

Screen Shot 2019-09-26 at 17 57 26

However we should look to remove moving the markup with JS and instead change the component markup so that the count message is inside govuk-form-group. This would be a breaking change, have created an issue: #1602

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1594 September 26, 2019 18:10 Inactive
@hannalaakso hannalaakso marked this pull request as ready for review September 30, 2019 06:55
@36degrees
Copy link
Contributor

Couple of minor typos in the commit messages:

3b52b27

We check whether the textarea is present before trying to its id to find the count message

814935a

It’s worth nothing that the code inside the block hasn’t been executed since [].

@hannalaakso hannalaakso force-pushed the character-count-associate-textarea branch from ff6c681 to 871669d Compare October 2, 2019 12:06
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1594 October 2, 2019 12:06 Inactive
@hannalaakso hannalaakso force-pushed the character-count-associate-textarea branch from 871669d to aeda9a2 Compare October 2, 2019 12:09
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1594 October 2, 2019 12:10 Inactive
@hannalaakso
Copy link
Member Author

Thanks @36degrees I've just fixed those

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.

Great cleanup, thanks @hannalaakso. Just a couple of minor things I've spotted.

@@ -5,6 +5,7 @@ import '../../vendor/polyfills/Element/prototype/classList'
function CharacterCount ($module) {
this.$module = $module
this.$textarea = $module.querySelector('.govuk-js-character-count')
if (this.$textarea) this.$countMessage = $module.querySelector('[id=' + this.$textarea.id + '-info]')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we tend to avoid inline if statements like this

Suggested change
if (this.$textarea) this.$countMessage = $module.querySelector('[id=' + this.$textarea.id + '-info]')
if (this.$textarea) {
this.$countMessage = $module.querySelector('[id=' + this.$textarea.id + '-info]')
}

return
}

// We move count message right after the field
// Kept for backwards compatibility
$textarea.insertAdjacentElement('afterend', this.$countMessage)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use $countMessage here? Alternatively, as we're currently only using $countMessage in one place, can we remove the local variable and just use this.$countMessage?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeps good catch 👍

The count message always exists (since 7b534bf). This PR makes the markup for it required,
instead of giving the option to add it with JS since the user needs to be informed
of the limit in the number of characters they can type in even if JS doesn’t run.

The count message DOM element is now defined at the top of the module with the other
required DOM elements. We check whether the textarea is present before using its id to
find the count message - the check prevents a DOM search error for `$textarea.id` if the
script is run without component markup being present present on the page.
This removes the branching logic and the code that added the count message markup dynamically in `createCountMessage()`.
It’s worth nothing that the code inside the block hasn’t been executed since 7b534bf.
As using the character count with the count message markup removed has never been part of the public API, removing this functionality
is not considered a breaking change.

The original code for checking/creating the count message markup [moves it in the DOM](/src/govuk/components/character-count/character-count.js@f481811#L104) to be inside
`govuk-form-group`. This was probably useful when the message markup was created dynamically. This PR retains the bit of code that
moves the element for backwards compatibility. It’s important that the count message markup is inside `govuk-form-group` so that it gets
wrapped within the error block. However we should look to remove moving it with JS and instead change the component markup so that
the count message markup is inside `govuk-form-group`. This would be a breaking change so shall leave it for now and create an issue.
Count message markup is now required so this check is not necessary.
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, thanks @hannalaakso

@hannalaakso hannalaakso merged commit 0db62d2 into master Oct 7, 2019
@hannalaakso hannalaakso deleted the character-count-associate-textarea branch October 7, 2019 15:04
36degrees added a commit that referenced this pull request Nov 11, 2019
This was accidentally added to the changelog for 3.3.0 after the release had been made – we've yet to release it, so it should be included in the unreleased section.
36degrees added a commit that referenced this pull request Nov 11, 2019
m-green pushed a commit that referenced this pull request Nov 13, 2019
This was accidentally added to the changelog for 3.3.0 after the release had been made – we've yet to release it, so it should be included in the unreleased section.
@NickColley NickColley added this to the v3.4.0 milestone Nov 13, 2019
@36degrees 36degrees mentioned this pull request Nov 19, 2019
peteryates added a commit to x-govuk/govuk-form-builder that referenced this pull request Nov 22, 2019
The recent upgrade to the GOV.UK Design System 3.4.0 caused a bug where
the JavaScript character/word counters no longer worked. The Design
System now requires that the descriptive text is set manually[0], which
will improve the experience for users who have disabled JavaScript.

alphagov/govuk-frontend#1594

Fixes #70
peteryates added a commit to x-govuk/govuk-form-builder that referenced this pull request Jan 9, 2020
The recent upgrade to the GOV.UK Design System 3.4.0 caused a bug where
the JavaScript character/word counters no longer worked. The Design
System now requires that the descriptive text is set manually[0], which
will improve the experience for users who have disabled JavaScript.

alphagov/govuk-frontend#1594

Fixes #70
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants