Tidy up and refactor the Character Count JavaScript #2807
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This abstraction isn't useful as the defaults can't be overridden anywhere, the two properties are only used once, and it means we need to jump around the code to understand what's going on.
We already read all of the `data-*` attributes into the `this.options` object, so use them instead of using `getAttribute`.
This also avoids implying that `this.options.maxwords` and `$module.getAttribute('data-maxwords')` are different things – they just represent the same data attribute on the module in different ways.
Avoid the indirection introduced by the countAttribute variable by setting this.maxLength conditionally based on whether `this.options.maxwords` is set.
Merge with the return statement if neither maxwords nor maxlength are set.
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-2807
domoscargin
approved these changes
Aug 24, 2022
| @@ -1,5 +1,5 @@ | |||
| import '../../vendor/polyfills/Function/prototype/bind.mjs' | |||
| import '../../vendor/polyfills/Event.mjs' // addEventListener and event.target normaliziation | |||
| import '../../vendor/polyfills/Event.mjs' // addEventListener and event.target normalization | |||
There was a problem hiding this comment.
I could be super pedantic and say this should even be "normalisation"...
| @@ -10,7 +10,9 @@ function CharacterCount ($module) { | |||
| this.lastInputTimestamp = null | |||
| } | |||
|
|
|||
| // Initialize component | |||
| /** | |||
| * Initialise component | |||
| * fields by directly changing its `value`. These changes don't trigger events | ||
| * in JavaScript, so we need to poll to handle when and if they occur. | ||
| * | ||
| * Once the and a keyup event hasn't been detected for at least 1000 ms (1s), |
There was a problem hiding this comment.
Suggested change
| * Once the and a keyup event hasn't been detected for at least 1000 ms (1s), | |
| * Once the keyup event hasn't been detected for at least 1000 ms (1s), |
domoscargin
reviewed
Aug 24, 2022
| /** | ||
| * Bind change events | ||
| * | ||
| * Set up events on the $textarea so that the counts update when the user |
There was a problem hiding this comment.
This makes it sound like internal counts are updating I think.
"... count messages update..."?
Also rename some functions to clarify what they actually do: - `formattedUpdateMessage` => `getCountMessage` - `checkIfValueChanged` => `updateIfValueChanged` Rename `element` to `$element` in the `getDataset` method to reflect the fact that it's an HTMLElement. Return directly from `count` function to avoid the unneccesary `length` variable.
Try to put the methods in a logical order so that methods that call each other are in close proximity and roughly in the order that they will be called.
36degrees
force-pushed
the
character-count-tidy
branch
from
1d9fa8b
to
67ef4a1
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-2807
domoscargin
approved these changes
Aug 25, 2022
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-pr-2807
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tidying this up with the aim of making it easier to set config (e.g. maxwords, threshold) by passing a config object at initialisation in the future.
Suggest reviewing commit by commit as quite a lot of the code has been moved around.