-
Notifications
You must be signed in to change notification settings - Fork 22
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: [DHIS2-17770] Org unit contextualization in self contained widgets #3720
feat: [DHIS2-17770] Org unit contextualization in self contained widgets #3720
Conversation
5f24da6
to
25a3c5a
Compare
…_OrgUnitContextualizationInSelfContainedWidgets
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.
Hey @henrikmv,
I didn't see any changes to the organizational unit in the stages&events widget as mentioned in the Jira ticket. The places where I saw the tooltip with the ancestors were the enrollment and profile widget. Did the scope of the feature change?
I also added a few comments and questions throughout the code.
Thanks!
src/core_modules/capture-core/components/WidgetProfile/WidgetProfile.component.js
Outdated
Show resolved
Hide resolved
src/core_modules/capture-core/components/WidgetEnrollment/WidgetEnrollment.component.js
Outdated
Show resolved
Hide resolved
interpolation: { escapeValue: false }, | ||
})} | ||
<span> | ||
{i18n.t('Started at ')} |
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.
Does it make sense to keep the orgUnitName
as part of the translatable string? In other languages, the order of the words can be different than in English. One suggestion is to pass an optional prop to TooltipOrgUnit
and to display it instead of {orgUnitName}
. Something like this:
<TooltipOrgUnit
orgUnitName={orgUnitName}
ancestors={ancestors}
tooltip={i18n.t('Started at {{orgUnitName}}', {
orgUnitName,
interpolation: { escapeValue: false },
})}
/>
// inside TooltipOrgUnit.component.js
<Tooltip content={orgUnitNameFullPath} openDelay={400}>
<span style={{ textDecoration: 'underline dotted' }}>
{tooltip ? tooltip : orgUnitName}
</span>
</Tooltip>
This change will apply the dotted line to the whole label. WYDT? Thanks!
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.
Talked to Henrik about this one. I agree it's important to be conscious about this problem, but in this case I suggest we simplify as you could really think of the string as two columns ("started at" and then the org unit name).
EDIT: Let's add a colon to be 100% sure (Started at: Ngelehun CHC)
src/core_modules/capture-core/components/Tooltips/TooltipOrgUnit/TooltipOrgUnit.component.js
Show resolved
Hide resolved
src/core_modules/capture-core/components/Tooltips/TooltipOrgUnit/TooltipOrgUnit.component.js
Outdated
Show resolved
Hide resolved
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.
Looks very close to done now 👍
src/core_modules/capture-core/metadataRetrieval/orgUnitName/orgUnitName.js
Outdated
Show resolved
Hide resolved
src/core_modules/capture-core/metadataRetrieval/orgUnitName/orgUnitName.js
Outdated
Show resolved
Hide resolved
src/core_modules/capture-core/components/Tooltips/TooltipOrgUnit/TooltipOrgUnit.component.js
Outdated
Show resolved
Hide resolved
src/core_modules/capture-core/components/Tooltips/TooltipOrgUnit/TooltipOrgUnit.component.js
Outdated
Show resolved
Hide resolved
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.
Great work! 🎉
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.
Well done, @henrikmv! 🎉
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.
Tested successfully on 2.42,2.41.2,2.40.6,2.39.7 versions
…onInSelfContainedWidgets
…/feat/DHIS2-17771_OrgUnitContextualizationInSelfContainedWidgets
# [101.5.0](v101.4.1...v101.5.0) (2024-09-24) ### Features * [DHIS2-17770] Org unit contextualization in self contained widgets ([#3720](#3720)) ([562b03a](562b03a))
🎉 This PR is included in version 101.5.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
DHIS2-17770
OrgUnitName.js
to include ancestors.DiszpKrYNg8: Object { displayName: "Ngelehun CHC", ancestor: "YuQRtpLP10I" }
.ancestor contains the id to the parent orgUnit in the orgUnit tree. With this structure for the cache, we are storing information about an orgUnit only once.
Please see the comment on Jira for more information about the cache structure.
Note: Wrong issue number on branch. The correct number is
DHIS2-17770