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 on edit document page #166

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/assets/stylesheets/application.scss
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ $govuk-global-styles: true;
@import "components/markdown-toolbar";
@import "components/summary";
@import "components/metadata";
@import "components/character-count";

@import "utilities/display";
23 changes: 23 additions & 0 deletions app/assets/stylesheets/components/_character-count.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
@import "../../../node_modules/govuk-frontend/settings/all";
@import "../../../node_modules/govuk-frontend/helpers/all";
@import "../../../node_modules/govuk-frontend/tools/all";

.govuk-textarea[data-module="character-count"],
.govuk-input[data-module="character-count"] {
display: block;
margin-bottom: govuk-spacing(1);
}

.govuk-textarea--error[data-module="character-count"],
.govuk-input--error[data-module="character-count"] {
padding: govuk-spacing(1) - 2; // stop a "jump" when width of border changes
}

.govuk-character-count__message {
margin-top: govuk-spacing(1);
margin-bottom: 0;
}

.govuk-character-count__message--disabled {
visibility: hidden;
}
166 changes: 166 additions & 0 deletions app/javascript/components/character-count.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
function CharacterCount ($module) {
this.$module = $module
}

// Initialize component
CharacterCount.prototype.init = function (options) {
// Check for module
var $module = this.$module
if (!$module) {
return
}

// Read options set using dataset ('data-' values) and merge them with options
var dataset = this.getDataset($module)
this.options = this.merge(options || {}, dataset)

// Read the limit attribute
var countAttribute = 'data-maxlength'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this variable now?


// Set the element limit
if ($module.getAttribute) {
this.maxLength = $module.getAttribute(countAttribute)
} else {
return
}

// Generate and reference message
var boundCreateCountMessage = this.createCountMessage.bind(this)
this.countMessage = boundCreateCountMessage()

// If there's a maximum length defined and the count message was successfully applied
if (this.maxLength && 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.

Again, can we return early here instead of having the block?

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

// Bind event changes to the textarea
var boundChangeEvents = this.bindChangeEvents.bind(this)
Copy link
Contributor

Choose a reason for hiding this comment

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

How come we need a variable here?

boundChangeEvents()

// Update count message
var boundUpdateCountMessage = this.updateCountMessage.bind(this)
boundUpdateCountMessage()
}
}

// Fills in default values
CharacterCount.prototype.merge = function (obj) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be easier to just manually merge attributes? I think there are only two of them ;-)

for (var i = 1; i < arguments.length; i++) {
var def = arguments[i]
for (var n in def) {
if (obj[n] === undefined) obj[n] = def[n]
}
}
return obj
}

CharacterCount.prototype.getDataset = function (element) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, would it be easier to just fish these out manually if there aren’t many?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or perhaps extract this to somewhere more general as it looks like it could be generally useful :-)

var dataset = {}
var attributes = element.attributes
if (attributes) {
for (var i = 0; i < attributes.length; i++) {
var attribute = attributes[i]
var match = attribute.name.match(/^data-(.+)/)
if (match) {
dataset[match[1]] = attribute.value
}
}
}
return dataset
}

// Generate count message and bind it to the input
// returns reference to the generated element
CharacterCount.prototype.createCountMessage = function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, wouldn’t it be easier to just the character count as actual HTML like contextual guidance?

var countElement = this.$module
var elementId = countElement.id
// 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) {
countElement.insertAdjacentHTML('afterend', '<span id="' + elementId + '-info" class="govuk-hint govuk-character-count__message" aria-live="polite"></span>')
this.describedBy = countElement.getAttribute('aria-describedby')
this.describedByInfo = this.describedBy + ' ' + elementId + '-info'
countElement.setAttribute('aria-describedby', this.describedByInfo)
countMessage = document.getElementById(elementId + '-info')
}
return countMessage
}

// Bind input propertychange to the elements and update based on the change
CharacterCount.prototype.bindChangeEvents = function () {
var $module = this.$module
$module.addEventListener('keyup', this.updateCountMessage.bind(this))

// Bind focus/blur events to start/stop polling
$module.addEventListener('focus', this.handleFocus.bind(this))
$module.addEventListener('blur', this.handleBlur.bind(this))
}

// Speech recognition software such as Dragon NaturallySpeaking will modify the
// 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.
CharacterCount.prototype.checkIfValueChanged = function () {
if (!this.oldValue) this.oldValue = ''
if (this.$module.value !== this.oldValue) {
this.oldValue = this.$module.value
var boundUpdateCountMessage = this.updateCountMessage.bind(this)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a variable here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm...so is this.updateCountMessage.bind(this)() not equivalent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope

boundUpdateCountMessage()
}
}

// Update message box
CharacterCount.prototype.updateCountMessage = function () {
var countElement = this.$module
var options = this.options
var countMessage = this.countMessage

// Determine the remaining number of characters/words
var currentLength = countElement.value.length
var maxLength = this.maxLength
var remainingNumber = maxLength - currentLength

// Set threshold if presented in options
var thresholdPercent = options.threshold ? options.threshold : 0
var thresholdValue = maxLength * thresholdPercent / 100
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't understand what this threshold percent thing is for - how does it relate to the UI?

if (thresholdValue > currentLength) {
countMessage.classList.add('govuk-character-count__message--disabled')
} else {
countMessage.classList.remove('govuk-character-count__message--disabled')
}

// Update styles
if (remainingNumber < 0) {
countElement.classList.add('govuk-textarea--error')
countMessage.classList.remove('govuk-hint')
countMessage.classList.add('govuk-error-message')
} else {
countElement.classList.remove('govuk-textarea--error')
countMessage.classList.remove('govuk-error-message')
countMessage.classList.add('govuk-hint')
}

// Update message
var charVerb = 'remaining'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these variables?

var charNoun = 'character'
var displayNumber = remainingNumber
charNoun = charNoun + ((remainingNumber === -1 || remainingNumber === 1) ? '' : 's')

charVerb = (remainingNumber < 0) ? 'too many' : 'remaining'
displayNumber = Math.abs(remainingNumber)

countMessage.innerHTML = 'You have ' + displayNumber + ' ' + charNoun + ' ' + charVerb
}

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

CharacterCount.prototype.handleBlur = function () {
// Cancel value checking on blur
clearInterval(this.valueChecker)
}

export default CharacterCount
2 changes: 2 additions & 0 deletions app/javascript/packs/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import restrict from '../restrict'
import editDocumentForm from '../documents/edit'
import MarkdownEditor from '../components/markdown-editor'
import ContextualGuidance from '../components/contextual-guidance'
import CharacterCount from '../components/character-count'
import Raven from 'raven-js'

var $sentryDsn = document.querySelector('meta[name=sentry-dsn]')
Expand All @@ -25,6 +26,7 @@ if ($sentryDsn && $sentryCurrentEnv) {

restrict('edit-document-form', editDocumentForm)
restrict('markdown-editor', ($el) => new MarkdownEditor($el).init())
restrict('character-count', ($el) => new CharacterCount($el).init())

// Initialise guidance at document level
var guidance = new ContextualGuidance()
Expand Down
Empty file.
4 changes: 4 additions & 0 deletions app/views/documents/edit.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
name: "document[title]",
value: @document.title,
data: {
"module": "character-count",
"maxlength": "65",
"contextual-guidance": "document-title-guidance"
}
} %>
Expand Down Expand Up @@ -55,6 +57,8 @@
value: @document.summary,
rows: 4,
data: {
"module": "character-count",
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"maxlength": "160",
"contextual-guidance": "document-summary-guidance"
}
} %>
Expand Down