Add character count on edit document page #166
Conversation
alex-ju
force-pushed
the
add-character-count
branch
from
2edf9ba
to
d104109
Compare
There was a problem hiding this comment.
I had some comments on the CharacterCount component but they seem less relevant knowing that it is being reviewed as part of alphagov/govuk-frontend#959
I wonder in cases where we're using stuff we're waiting on govuk_frontend it'd make sense that we just use a branch in our package.json? that might stop repeating the code (but could cause it's own different problems)
It'd be useful to have more context in the commit message as well to explain where the character count stuff was from as going through a git history we don't have the PR message.
Also add this looks really cool 👍
|
@kevindew yes, I rely much on the Github PR message instead of commit message. I should mirror these because different people have different ways of reviewing. Using a branch from npm is a good idea, unfortunately content-publisher doesn't yet consume frontend directly, but via govuk_publishing_components gem, but after updating it to consume govuk-frontend directly I'll take the branch approach. |
|
@kevindew Should this have a small spin-off story for us to update the validations? |
There was a problem hiding this comment.
So this seems like quite a lot of code to generate single line of text - sorry for the bluntness, but it is a lot of logic. One reason I think it's so long is that we've actually got two components here: a character count and a word count, which is also generating a lot of branching.
| @@ -55,6 +57,8 @@ | |||
| value: @document.summary, | |||
| rows: 4, | |||
| data: { | |||
| "module": "character-count", | |||
There was a problem hiding this comment.
It looks like this kind of technique doesn’t extend if we wanted to add more modules to this element - would it be better to attach it in a similar way as contextual guidance?
There was a problem hiding this comment.
Yes, that is true, this method allows only one module per container, but generally I would avoid initialising multiple scripts on the same component as they may hijack each other's events. One script per component is consistent with how JavaScript is attached to all the components that are coming from govuk-frontend. If there are scripts acting at a document level, as it was the case of the contextual guidance, it makes sense for that script to use a data-[component-name] attribute selector or a .js-[component-name] class selector as we don't want to overlap with a component model and we want to be able to identify which script manipulates/affects the component. This is one of those things that I should document and explain at the project level.
| var countAttribute = (this.options.maxwords) ? this.defaults.wordCountAttribute : this.defaults.characterCountAttribute | ||
|
|
||
| // Set the element limit | ||
| if ($module.getAttribute) { |
There was a problem hiding this comment.
Should we just quit early here instead of having another if later on?
There was a problem hiding this comment.
Yes, we can quit early here. Maybe with a console message to mention an attribute is missing. Will update.
| this.options = this.merge(options || {}, dataset) | ||
|
|
||
| // Determine the limit attribute (characters or words) | ||
| var countAttribute = (this.options.maxwords) ? this.defaults.wordCountAttribute : this.defaults.characterCountAttribute |
There was a problem hiding this comment.
It looks like we’re doing this on characters - why are we supporting words?
There was a problem hiding this comment.
It was part of the original script which allows both characters and words. I pulled this in 'as is' because this is how it will come from govuk-frontend. Can remove the maxwords for now.
| } | ||
|
|
||
| // Fills in default values | ||
| CharacterCount.prototype.merge = function (obj) { |
There was a problem hiding this comment.
Would it be easier to just manually merge attributes? I think there are only two of them ;-)
| return obj | ||
| } | ||
|
|
||
| CharacterCount.prototype.getDataset = function (element) { |
There was a problem hiding this comment.
Again, would it be easier to just fish these out manually if there aren’t many?
There was a problem hiding this comment.
Or perhaps extract this to somewhere more general as it looks like it could be generally useful :-)
| if (!this.oldValue) this.oldValue = '' | ||
| if (this.$module.value !== this.oldValue) { | ||
| this.oldValue = this.$module.value | ||
| var boundUpdateCountMessage = this.updateCountMessage.bind(this) |
There was a problem hiding this comment.
Why do we need a variable here?
There was a problem hiding this comment.
We need to store and execute the bounded function so no matter how and when it is called (on which component's instance), is called with a particular this (in our case our component's instance).
There was a problem hiding this comment.
Hmm...so is this.updateCountMessage.bind(this)() not equivalent?
| var remainingNumber = maxLength - currentLength | ||
|
|
||
| // Set threshold if presented in options | ||
| var threshold = 0 |
There was a problem hiding this comment.
You could make this a ternary - it seems to be the pattern used in the code above.
| if (options.threshold) { | ||
| threshold = options.threshold | ||
| } | ||
| var thresholdValue = maxLength * threshold / 100 |
There was a problem hiding this comment.
Er, where’s the magic 100 coming from?
There was a problem hiding this comment.
threshold is a percentage value
| } | ||
|
|
||
| // Generate and reference message | ||
| var boundCreateCountMessage = this.createCountMessage.bind(this) |
There was a problem hiding this comment.
How come we need a variable here?
| $module.setAttribute('data-maxlength', $module.maxLength) | ||
|
|
||
| // Bind event changes to the textarea | ||
| var boundChangeEvents = this.bindChangeEvents.bind(this) |
There was a problem hiding this comment.
How come we need a variable here?
I wouldn't judge or measure the code by how much the users sees 😉 It may look a bit complex because it deals with both scenarios (character and word count). This is already a cleaned-up version without the highlight functionality which in my opinion contains all the complexity. Updated:
Regarding Tried to reply the other questions inline, please let me know if I missed any. |
There was a problem hiding this comment.
This is looking better, but I think there's still some cleanup to do. I'm particularly concerned about the dataset get/merge functions. While I can see they're generic, they're also long and complicated and we could replace them with something much shorter and easier to read. Can we have a go at doing this and see how it looks?
| this.options = this.merge(options || {}, dataset) | ||
|
|
||
| // Read the limit attribute | ||
| var countAttribute = 'data-maxlength' |
There was a problem hiding this comment.
Do we need this variable now?
| this.countMessage = boundCreateCountMessage() | ||
|
|
||
| // If there's a maximum length defined and the count message was successfully applied | ||
| if (this.maxLength && this.countMessage) { |
There was a problem hiding this comment.
Again, can we return early here instead of having the block?
|
|
||
| // Set threshold if presented in options | ||
| var thresholdPercent = options.threshold ? options.threshold : 0 | ||
| var thresholdValue = maxLength * thresholdPercent / 100 |
There was a problem hiding this comment.
I still don't understand what this threshold percent thing is for - how does it relate to the UI?
| } | ||
|
|
||
| // Update message | ||
| var charVerb = 'remaining' |
There was a problem hiding this comment.
Do we need these variables?
|
Closing this as I consider unproductive to extract and branch code from a component that will replace this one. Thanks @benthorner for reviewing! For now we can use a hint text to state the maximum limit and rely on server side for validation. Will bring the character count component from |
This PR adds a character count to the Title (65 chars) and Summary (160 chars) fields on the edit document page. The code for this PR is based on the upcoming Character count component in govuk-frontend.
Trello card