-
Notifications
You must be signed in to change notification settings - Fork 2
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
Austenem/CAT-873 add donor age messaging #3548
Conversation
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.
Just a few minor tweaks, great work otherwise! Also, would it be possible to add this logic to the MetadataTable
component as well to make sure we cover it wherever necessary?
context/app/static/js/components/entity-tile/EntityTileBody/EntityTileBody.tsx
Show resolved
Hide resolved
context/app/static/js/components/entity-tile/EntityTileBody/EntityTileBody.tsx
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.
I should have included this in my PR, but we need to make mapped_metadata
potentially undefined in the Donor
type.
I can definitely do that, but the metadata table already has this language in the top info banner - I'd be in favor of replacing that with an inline tooltip like the rest of the messaging, or including both. @tsliaw? |
@austenem I would be fine with a tooltip for the metadata table to keep it consistent. Let's remove the messaging in the info textbox. I think the messaging appears in enough places to be clear about what's going on in this scenario. |
context/app/static/js/components/detailPage/MetadataTable/MetadataTable.tsx
Outdated
Show resolved
Hide resolved
{fieldValue.join(' / ')} | ||
<DonorAgeTooltip donorAge={fieldValue.join(' / ')} /> |
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.
Will this be able to be converted with Number
?
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, because the fieldValue
array only contains one element - but it might be a bit safer to take only the first element of fieldValue
.
@@ -55,7 +55,7 @@ function getByPath(hitSource, field) { | |||
return ( | |||
<Stack direction="row"> | |||
{fieldValue.join(' / ')} | |||
<DonorAgeTooltip donorAge={fieldValue.join(' / ')} /> | |||
<DonorAgeTooltip donorAge={fieldValue[0]} /> |
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 should check that fieldValue
is non-empty.
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!
Re-ran cypress. Merge once passing. |
Summary
Add messaging where donors with ages > 89 are displayed on search and detail pages, and cap age filter on search page at 90 years.
Design Documentation/Original Tickets
CAT-873 Jira ticket
Testing
Tested for both 90+ aged donors.
Screenshots/Video
Search page:
Dataset detail page:
Donor detail page:
Checklist
CHANGELOG-your-feature-name-here.md
is present in the root directory, describing the change(s) in full sentences.