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

character-count displays undefined when maxlength and maxwords are both undefined #1725

Closed
omair-ali opened this issue Feb 6, 2020 · 7 comments

Comments

@omair-ali
Copy link

text: 'You can enter up to ' + (params.maxlength or params.maxwords) + (' words' if params.maxwords else ' characters'),

When maxlength and maxwords are both undefined, the following message is observed:
You can enter up to undefined characters

@NickColley
Copy link
Contributor

Hello @omair-ali,

The maxlength or maxwords options are required for this component to work. So you will need to define these if you want the component to work as expected.

Is there a usecase you've come across where you would want to use this component without either of these required options?

@NickColley NickColley added the awaiting triage Needs triaging by team label Feb 6, 2020
@omair-ali
Copy link
Author

Hi @NickColley,
Thanks for reviewing this request. The reason we have raised this is because there is no requirement / validation criteria. The users of this component can pass in undefined and nothing restricts them or invalidates the input suggesting either has to be defined.
We are looking at this from our Scala Twirl based lib which mirrors this. In order to have a parity with govuk-frontend we think if this requirement is incorporated here then our mirrored lib can restrict our client too from not providing one of the values. If we are going to enforce this then our lib will start diverging from govuk-frontend. HEnce, we think this could be a useful addition here too. Please advise, thanks

@NickColley
Copy link
Contributor

With our Nunjucks templates we don't have a way to validate required fields and display an error or warning to our users.

The required options are documented in the 'Nunjucks tab > Nunjucks macro options' section the website.

We recommend that people validate their data before they pass it to inputs rather than relying on the templates themselves which contain minimal logic.

If we were to change this we'd have to do this for all templates not just the character count component.

We're open to any suggestions that could make it easier for you to port these templates but I'm not aware of a way to do validation of required fields within Nunjucks templates.

Interested to hear your thoughts.

@timpaul timpaul removed the awaiting triage Needs triaging by team label Feb 10, 2020
@omair-ali
Copy link
Author

@natcarey any thoughts?

@nataliecarey
Copy link
Contributor

Thanks @NickColley.

So in HMRC we're building a port of govuk-frontend, I know that OPSS are doing the same. Our testing strategy is based on an a black-box comparison of the html generated by the components in govuk-frontend with the html generated by our port when using the same component and the same input.

If we output the same HTML from the same input Parameters then the CSS/JS/Assets will all be compatible.

With this approach I advocate for "feature parity" and "bug parity" - if we come across something we feel is a bug (in this case telling the user "You can enter up to undefined words") then we should match the behaviour. If we act differently in cases we perceive as bugs then we no longer have an unopinionated port.

When we encounter things we perceive as bugs we intend to match the behaviour but raise the issue. So without a change from GDS we'll implement the "feature" that leaving both parameters empty produces "You can enter up to undefined words".

One of the reasons we like to raise these things is that we know we're quite intensive users and we might spot things that were undesirable.

Outcomes of this are:

  • GDS change and HMRC change in-step
  • GDS don't change and HMRC match the behaviour
  • GDS don't change and HMRC decide a different behaviour for this case

In future we can raise these kind of issues or not raise them, we're always happy to raise solutions too like we did with #1618

@NickColley
Copy link
Contributor

Heya @natcarey :)

As far as I know this behaviour is shared by all components, we assume that users will input require options since they would not work otherwise.

As it stands I'm not sure of a way we can make examples like this throw errors or let users know they failed to use required options.

One thing I have considered is that the component does not output a message if the user fails to include these options but that could arguably be less helpful than an 'undefined' message as at least an 'undefined' message provides feedback that something is not right.

If you have a suggestion of what approach we should consider across all components when a user fails to input require options we'd be open to it.

So if you could help me consider other options that'd be helpful, look forward to your thoughts.

@36degrees
Copy link
Contributor

I believe this was fixed in v4.4.0 – creating a character count with no maxlength or maxwords now results in a character count with no count message (effectively just a textarea).

@36degrees 36degrees added this to Backlog 🗄 in Design System Sprint Board via automation Nov 29, 2022
@36degrees 36degrees moved this from Backlog 🗄 to Done 🏁 in Design System Sprint Board Nov 29, 2022
@36degrees 36degrees added this to the v4.4.0 milestone Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

6 participants