ing-bank / lion

Fundamental white label web component features for your design system.
https://lion.js.org
MIT License
1.77k stars 295 forks source link

[lion-input-iban] Being able to redefine an error message from a default validator #1783

Open paulcodiny opened 2 years ago

paulcodiny commented 2 years ago

Expected behavior

A dutch label of the default error: "Vul een geldig Rekeningnummer in."

Actual Behavior

A dutch label of the default error: "Vul een geldig(e) Rekeningnummer in."

Additional context

We use an lion-input-iban. It works great and looks nice but there is a small thingy that caught our eye. It has a default label, in Dutch it's Rekeningnummer. According to Dutch grammar in this case an adjective before does not need to have "e". However, because input allows a label to be passed, an error message takes into account case when an adjective "geldig" should have "e".

One of the ways of making it a bit nicer is to be able to pass messages to the default validators.

Another one is maybe that component has two different types of error messages: when it knows the label because the default label is used or generic error when a label is passed.

I'm curious about your opinion on this.

This is not critical. The component is already working good for us. More like a question

tlouisse commented 2 years ago

Thanks for the request. Imho this is indeed a missing feature for a long time. Creating this would technically be easy. Only hard thing is a nice api :)

So for instance we could do it via:

<lion-input-iban .defaultValidatorConfigs="${{IsIBAN: {getMessage: () => 'Vul een geldig rekeningnummer in'}}}">

This (keyed by name) would then only work if we didn't allow two instances of the same validator. I think we avoided this possibility, but there were some edge cases here maybe. Maybe you remember @jorenbroekema ?

Open for api suggestions...

ryubro commented 8 months ago

Hi,

For this exact case, shouldn't the following work?

IsIBAN.getMessage = () => 'Vul een geldig Rekeningnummer in.';

html`
  <lion-input-iban .modelValue=${'NL20INGB0001234567XXXX'}></lion-input-iban>
`;

Edit: after reading the first post again, I noticed the expected message is ... geldig ... not ... geldige ..., hence updated.

jorenbroekema commented 8 months ago

Maybe like so:

// this is not set by the consumer btw, it's just the predefined getMessage for Dutch on (IsIBAN) from lion
// consumer can override it though of course, if they need to
IsIBAN.getMessage = ({ usesDefaultLabel, article }) => `Vul een geldig${usesDefaultLabel ? '' : (article ?? '(e)') } {fieldName} in.`;

html`
  <lion-input-iban
    .modelValue=${'NL20INGB0001234567XXXX'}
  ></lion-input-iban>
`;
// Vul een geldig Rekeningnummer in.

html`
  <lion-input-iban
    .label=${'Banknummer'}
    .article=${''} 
    .modelValue=${'NL20INGB0001234567XXXX'}
  ></lion-input-iban>
`;
// Vul een geldig Banknummer in.

html`
  <lion-input-iban
    .label=${'Banknummer'}
    .modelValue=${'NL20INGB0001234567XXXX'}
  ></lion-input-iban>
`;
// Vul een geldig(e) Banknummer in.

That way, if the default label is used, we know the article is "the" and thus no need for "e" or "(e)". If a custom label is used, we can use the article if that's passed by consumer (e.g. "" for "het" or "e" for "de"), otherwise we fallback to the "e" being optional "(e)", because we're not sure about the article

Not sure how this works for other languages though.. but imo the two things we want to achieve is:

And doing this in a way where the getMessage doesn't need to be overwritten, you can do this of course but that's a bit overkill if we just want to make the article smarter.

Only question for me is how do we, technically, pass that article logic to translations files similar to how we pass stuff like fieldName, params etc., it's been too long ago for me to know if what im proposing is possible, maybe: https://github.com/ing-bank/lion/blob/master/packages/ui/components/input-iban/translations/nl.js#L3C14-L3C47

{
  IsIBAN: `Vul een geldig{article, select,
    defaultLabel {e}
    undefined {(e)}
    other {{article}}
  } {fieldName} in.`,
}

and ensure the article is determined e.g. here: https://github.com/ing-bank/lion/blob/master/packages/ui/components/form-core/src/validate/ValidateMixin.js#L764C43-L764C54 , I'm guessing in this method we know about label and article props and whether default label was used so we could do:

const message = await validator._getMessage({
  modelValue: this.modelValue,
  formControl: this,
  fieldName,
  outcome,
  // note 1: usesDefaultLabel prop is just pseudo code..
  // note 2: 'defaultLabel' acts as a special key here, so the article cannot be this special key..
  article: this.article ?? (this.usesDefaultLabel ? 'defaultLabel' : undefined)
});
ryubro commented 8 months ago

I don't speak Dutch so I might be mistaken, but having .article in the input component level doesn't feel right to me. Error message format should be Validator's concern, and shouldn't spill over to input, IMHO.

Practically something like this is possible:

const getMessageWithArticleInMind =
  (article) => 
  aysnc ({ fieldName }) => `Vul een geldig${article ?? '(e)'} ${fieldName} in.`;

html`
  <lion-input-iban
    .modelValue=${'NL20INGB0001234567XXXX'}
    .validators=${[new IsIBAN(undefined, { getMessage: getMessageWithArticleInMind('') })]}
  ></lion-input-iban>
`;
// Vul een geldig Banknummer in.

html`
  <lion-input-iban
    .modelValue=${'NL20INGB0001234567XXXX'}
    .validators=${[new IsIBAN(undefined, { getMessage: getMessageWithArticleInMind() })]}
  ></lion-input-iban>
`;
// Vul een geldig(e) Banknummer in.

But internally two validation messages are created, one passed as .validator and the other one as the default validator from the input-iban constructor: https://github.com/ing-bank/lion/blob/79a64674f8f644efe0468e5518c8cd5499c8a7cd/packages/ui/components/input-iban/src/LionInputIban.js#L11-L18

Due to that input-iban shows only one error message and some internal logic how messages are sorted, we only see the passed one, but not entirely sure if it's legit enough to rely on.

jorenbroekema commented 8 months ago

having .article in the input component level doesn't feel right to me. Error message format should be Validator's concern, and shouldn't spill over to input, IMHO.

That's a valid point but keep in mind that label isn't always used as a fieldName in validation messages. It is the default, but often folks will set the fieldName property to influence what the validation message looks like, so a ValidationMixin concern has already spilled over into the input props, so having a fieldNameArticle prop next to it doesn't feel so strange if you consider this.

Practically something like this is possible

Yeah this is totally valid, just feels a little bit overkill if the only override we want to do is the article, not the whole message. I'm still not sure how that would go for other languages e.g. French has feminine/masculine which could translate to "petit" versus "petite". So in this case, how can we pass a fieldNameArticle that is also contextualized for multiple locales? What does this API look like?

not entirely sure if it's legit enough to rely on

As far as I remember default validators and user passed validators are indeed sorted appropriately and this is well-tested behavior: https://github.com/ing-bank/lion/blob/master/packages/ui/components/form-core/test-suites/ValidateMixin.suite.js This suite runs for many form components, most of which have default validators, and the custom validator tests pass for all of them.

ryubro commented 8 months ago

Ah, now I see. So the form of article depends on the content of the fieldName, something like a minute and an hour.

Regarding the API, this reminds me the select syntax in ICU message but of course this assumes that we know all the possible values. Probably we should use ChatGPT ;-) But personally I wouldn't mind geldig(e) if it's only as bad as a(n) hour.

As far as I remember default validators and user passed validators are indeed sorted appropriately and this is well-tested behavior: https://github.com/ing-bank/lion/blob/master/packages/ui/components/form-core/test-suites/ValidateMixin.suite.js

Thank you for the explanation and link to the tests. Indeed the order of the validators seems part of the spec.