-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat: Add constants.humanName to allow countries to have custom full name format #7812
base: develop
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
Your environment is deployed to https://ocrvs-6830-human-name-constant.opencrvs.dev |
@@ -978,6 +979,11 @@ const messagesToDefine: IConstantsMessages = { | |||
defaultMessage: `{registrationTargetDays} days - 1 year`, | |||
description: `Label for registrations within {registrationTargetDays} days to 1 year`, | |||
id: 'constants.withinTargetDaysTo1Year' | |||
}, | |||
humanName: { | |||
defaultMessage: `{lastName} {middleName} {firstName}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if one of them is missing? The middleName is probably optional in most cases. Then there's also incomplete declarations which might not contain the full name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rikukissa @Siyasanga From what I could gather, the intl formatMessage syntax doesn't really have a way of handling null values (i.e. absent values) rather it needs to be handled on the js level with multiple messages e.g.
if (firstName && middleName && lastName) {
return humanName // `{lastName} {middleName} {firstName}`
} else if (!middleName) {
return humanNameWithoutMiddleName // `{lastName} {firstName}`
}
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Zangetsu101 for this question. I was concerned about that as well, when I was manually testing this scenario yesterday it seemed to work just fine when all the placeholders
are passed as field in the values
, if the value is falsey
(null
,false
,undefined
except the number 0 which gets rendered as is) for a key
it gets omitted in the rendered message.
The only time it seemed to break is when one of the placeholders(firstName
,middleName
,lastName
) is missing from the values array which will result in the rendering: {lastName} {middleName} {lastName}
I just struggled to write tests to demostrate these scenarios because I couldn't figure out how to set different locales
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm so you're saying even if provide an empty string for the middleName part, it renders properly? I would have guessed that it would render something like this: John Doe
(Notice the double space in between) for the given value: { firstName: "John", middleName: '', lastName: "Doe" }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a country can achieve the proper format if we default empty values as null
than can be then matched against in the language string.
https://formatjs.github.io/docs/core-concepts/icu-syntax/
Happy path
Middle name missing (extra space)
Defining empty values as null
{first}{middle, select, null {} other { {middle}}} {last}
intl.formatMessage(constantsMessages.humanName, { | ||
firstName: user.name[0].firstNames, | ||
middleName: user.name[0].middleName, | ||
lastName: user.name[0].familyName | ||
})) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to have a utility function similar to the createNamesMap
, something like this:
function internationalizedName(name: HumanName[], locale: Locale): {
firstName: string
middleName: string
lastName: string
}
It would try to find the name in the given locale, falling back to 'en' in case it does not find it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is where I was getting confused. It looks like the translations are passed through from IntlProvider
and also pulled from country-config
, now passing the locale implies that we have access to all different translations and dynamically changes languages. Do you know where I can get this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just pass in intl.locale
to the function. As for how to resolve that, take a look at the current implementation of the createNamesMap
function, it should give you an idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really support names in multiple languages 🤔 If not, then I'd aim to simplify as much as we can. IMO best would be a component like
<HumanName name={user.name} />
component that could use the useIntl
hook. Passing the intl object as a function parameter feels like a little upside down
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rikukissa on the system user we only support one language, English and on the Birth and Death forms it looks like we only ask First names
and Surname
. Does this confirm that we don't support name in multiple languages for Birth
, Marriage
and Death
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that's a safe assumption
Thank for the feedback guys @rikukissa @Zangetsu101, I ended up going with the utility function approach because I needed just the localised name in some instances instead of the React.Node component. Please let me know if you have feedback |
46b6c1d
to
d1e6f6a
Compare
I think we can go with this approach. Removing multiple spaces using regex is better as we would have to handle quite a few edge cases otherwise |
c23ce35
to
41e9d75
Compare
To allow countries to have custom ordering for full names #6830
To ensure that we get the format from the country-config #6830
To make the name more usable we had to extract the name formatting logic into it's own function. #6830
Replace older logic to get the name which was based on an assumption that we support names in multiple languages #6830
We've found cleaner way to make the rendered name customizable for each country through client copy from country-config #6830
We need to update all the places where a citizen's name is being referenced to show it in the format that the country chooses #6830
720bc5f
to
d826a03
Compare
In order to allow countries to define their own full name format, we are adding a constants.humanName into the
client.csv
translations, which will then be used everywhere where we are displaying system users and citizen's name in the systems.#6830