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

Make CharacterCount use of attributes option consistent with other components #2893

Closed
4 tasks
romaricpascal opened this issue Oct 3, 2022 · 3 comments · Fixed by #4566
Closed
4 tasks

Comments

@romaricpascal
Copy link
Member

What

Update CharacterCount component so its attributes option sets attributes on the root tag of the component, instead of the textarea. To still allow setting attributes on the textarea, introduce a textareaAttributes.

This'll require a two step approach where:

  1. [Non breaking] We introduce the new textareaAttributes option and:
    • if present, assign attributes to the root tag
    • if absent, assign attributes to the textarea like we do now and warn users that the meaning of the attributes option will soon change and encourage using textareaAttributes
  2. [Breaking] Remove the current behaviour and always assign attributes to the root tag

Why

This'll make the component consistent with the rest of the other components which set attributes on their root tag, for ex. allowing them to set a lang attribute without having to wrap the component.

Who needs to work on this

Developers

Who needs to review this

Developers

Done when

  • New textareaAttributes option is introduced and used for picking on which tag attributes apply
  • Deprecation message is shown if only using the attributes
  • New behaviour is documented in the component options
  • Issue is created to remove the current behaviour and deprecation message
@36degrees
Copy link
Contributor

Related: #1649

@querkmachine
Copy link
Member

Given this is an issue with multiple Nunjucks parameters, would it be sensible to have a nested set of params rather than adding new top-level ones?

i.e. Instead of adding textareaAttributes, textareaClasses, textareaId, and so on, we do this?

{{ govukCharacterCount({

  // These apply to the textarea
  attributes: {},
  classes: '',
  id: '',
  ...

  // These apply to the container
  formGroup: {
    attributes: {},
    classes: '',
    id: '',
    ...
  }

}) }}

That would still be internally consistent (we use formGroup.classes to apply classes on many form input types) and—at first glance—would not require a breaking change.

@colinrotherham
Copy link
Contributor

This is now ready for review as part of #4566

I've gone with @querkmachine approach from #2893 (comment)

@colinrotherham colinrotherham moved this to Needs review 🔍 in GOV.UK Design System cycle board Feb 9, 2024
@colinrotherham colinrotherham moved this from Needs review 🔍 to Ready to release 🚀 in GOV.UK Design System cycle board Mar 6, 2024
@36degrees 36degrees added this to the [next] milestone Mar 11, 2024
@36degrees 36degrees moved this from Ready to release 🚀 to Done 🏁 in GOV.UK Design System cycle board Mar 26, 2024
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.

4 participants