-
Notifications
You must be signed in to change notification settings - Fork 13
Conversation
Looking at this now. |
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.
When I tested a file with 2 processing types (processing-type-2.csv) I saw duplicate facility types, which is expected based on our implementation
II was able to resolve this by adding some logic and renaming our existing list formatting helper
diff --git a/src/app/src/components/FacilityDetailSidebarExtended.jsx b/src/app/src/components/FacilityDetailSidebarExtended.jsx
index bad608f..666e9b1 100644
--- a/src/app/src/components/FacilityDetailSidebarExtended.jsx
+++ b/src/app/src/components/FacilityDetailSidebarExtended.jsx
@@ -84,8 +84,8 @@ const detailsSidebarStyles = theme =>
const formatAttribution = (createdAt, contributor) =>
`${moment(createdAt).format('LL')} by ${contributor}`;
-const formatIfList = value =>
- Array.isArray(value) ? value.map(v => <li>{v}</li>) : value;
+const formatIfListAndRemoveDuplicates = value =>
+ Array.isArray(value) ? [...new Set(value)].map(v => <li>{v}</li>) : value;
/* eslint-disable camelcase */
const formatExtendedField = ({
@@ -96,7 +96,7 @@ const formatExtendedField = ({
id,
formatValue = v => v,
}) => ({
- primary: formatIfList(formatValue(value)),
+ primary: formatIfListAndRemoveDuplicates(formatValue(value)),
secondary: formatAttribution(updated_at, contributor_name),
verified,
key: id,
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.
Other than the duplication this works as intended. If you like my suggested diff and include it, consider this approved.
@jwalgran Good catch. I forgot that having two processing types will now add two facility types. I'll integrate your diff. |
@jwalgran I implemented your fix, and did some more testing. In the case when two facilities have been matched, and they have the same processing or facility type, this is what it will look like: Do we want to remove duplicate values across extended field type entries (the current code only does it within the same entry)? A similar thing happens with product type as well: |
Handling the cases when a contributor submits the same extended field values multiple times is something that we should address, but is outside the scope of this PR since, as you mentioned, it is not just related to the processing type and facility type fields. I created #1626 to follow up on this. |
6bd42ae
to
80c5eb6
Compare
Show the processing type(s) and facility type(s) for a facility in the facility details sidebar. Also, reorder the extended fields in the sidebar so that processing type and facility type are next to each other. Refs #1618
80c5eb6
to
364bc36
Compare
Overview
Show the processing type(s) and facility type(s) for a facility in the facility details sidebar.
Also, reorder the extended fields in the sidebar so that processing type and facility type are next to each other.
Connects #1618
Demo
Testing Instructions
Checklist
fixup!
commits have been squashed