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

Allow character count config set via data attributes to be passed when initialising the component in JavaScript #2837

Closed
4 tasks
36degrees opened this issue Sep 12, 2022 · 6 comments · Fixed by #2883

Comments

@36degrees
Copy link
Contributor

36degrees commented Sep 12, 2022

What

The character count component currently uses data attributes to configure the component:

  • data-maxlength to determine the character limit displayed to the user
  • data-maxwords to determine the word count displayed to the user, if it makes more sense to count words than characters
  • data-threshold to determine a cut-off point at which the count message should be displayed

Update the component so that this config can also be passed as an object when initialising the component in JavaScript.

As per #1708 (comment), any config passed via data attributes takes precedence over config passed when initialising a component.

Why

This aligns the general concept of 'component config' with the approach we're using for internationalisation.

Who needs to work on this

Developers

Who needs to review this

Developers

Done when

  • The component can be configured by provided a config object when instantiating a CharacterCount object
  • The component can be configured when initing a CharacterCount object using initAll
  • If data attributes are present on the $module HTML element they override any config set via initialisation
  • Tests to cover the above
@romaricpascal
Copy link
Member

romaricpascal commented Sep 26, 2022

If data attributes are present on the $module HTML element they override any config set via initialisation

Fun bit with this component there's a bit more precedence than just data-attributes over JavaScript configuration. Internally, the component also gives precedence to maxwords over maxlength if both are set. The issue is one may be set up through data-attributes and the other through JavaScript config (eg. via initAll).

For example when running:

new CharacterCount($element, {maxwords: 150}).init();

with $element being the DOM node for:

<div class="govuk-character-count" data-module="govuk-character-count" data-maxlength="200">
  <!-- Omitted for clarity -->
</div>

In that case, I'd expect that the presence of data-maxlength would turn that specific character count into a field counting characters rather than words.

Just wanted to check that it's what we all expect of the component behaviour as that requires a slightly different merging strategy compared to the components we've added config to so far.

In terms of practicalities, I'm thinking of cloning and cleaning the JavaScript config of its maxlength or maxwords keys in case the data attributes contains one of these keys before merging it.

@36degrees
Copy link
Contributor Author

As long as there's a way to 'switch back' to character-counting mode I think we should do whatever's simplest – if we end up introducing a 'special case' for this component it'll limit our ability to move the config-merging logic into a base component later.

What if we made it so that you can disable the maxwords 'override' by setting it to false in the data attributes?

So this combination of JS config and data attributes would count characters:

new CharacterCount($element, {maxwords: 150}).init();
<div class="govuk-character-count" data-module="govuk-character-count" data-maxlength="200" data-maxwords="false">
  <!-- Omitted for clarity -->
</div>

@romaricpascal
Copy link
Member

This would cancel out the maxwords option indeed, but puts the work on the user to remember that somewhere, somehow, there's something initialising the component with maxwords already, so I'm not sure of that route.

Doing some pre-processing doesn't necessarily prevent abstracting the merging of configurations. We could for example pass the shared merging logic a function to do some pre-processing before merging, letting components cleanup, disambiguate similar keys or what not. I've got a Ruby example of such API in this library I worked on (do ... end blocks in Ruby are pretty much like passing a function as an argument).

@36degrees
Copy link
Contributor Author

OK.

I am wondering if we should revisit the API for the character count to try and avoid this being a problem. We could potentially have a config option which determines what 'type' of count to use (count: 'words' or count: 'characters') so the limit and the counting method are configured separately.

That would also set us up to allow a custom counting function to be passed in the future, closing #1364.

This might be something we could look at now – we would need to maintain the existing behaviour but we could mark parts of the current API (probably maxwords?) as deprecated and remove them in a future release.

Alternatively we could just write this up as something to revisit in the future.

@romaricpascal
Copy link
Member

Revisiting the API sounds a good idea long term. Not only it would solve that conflict between maxwords and maxlength and open to having a function for counting, I think there's also room for some subtle renaming of threshold as well (which doesn't really scream that it's the threshold for displaying the notice), and maybe even maxlength (to avoid possible confusion with the HTML attribute when not counting characters, which I guess is why the component has a maxwords for words).

This would obviously be quite a breaking change to the API, so we should put some deprecations in place. I guess whether we do this part of this chunk of work depends on whether we're planning further releases between the upcoming one and 5.0.

@36degrees
Copy link
Contributor Author

At the minute, I'm expecting that we'll start thinking about 5.0 once we've got this release out, so I'm not anticipating much in the way of further 4.x releases, unless there are things that come out of the remaining 'future improvements' sessions that we think we want to ship before 5.0, or we have any contributions that are ready to release before 5.0 (summary card, hide this page).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

2 participants