Destination Sample Record Card exports and updates precisely #3071
Conversation
ui/ui-components/__tests__/components/record/SampleRecordCard.tsx
Outdated
Show resolved
Hide resolved
| const canExportRecord = useMemo( | ||
| () => destinations?.find(({ id }) => id === destinationId), | ||
| () => !!destinations?.find(({ id }) => id === destinationId), |
There was a problem hiding this comment.
The canExportRecord flag seems to indicate whether to add the button or not. Would it make sense to check if there is a destinationId defined here and then avoid having to check below for destinationId to render the button or not?
There was a problem hiding this comment.
Adding a short-circuit here
| @@ -366,6 +366,176 @@ const SampleRecordCard: React.FC<SampleRecordCardProps> = ({ | |||
| setExporting(false); | |||
| }, [client, destinationId, record?.id]); | |||
|
|
|||
| const content = useMemo(() => { | |||
There was a problem hiding this comment.
Hard to compare with the one below because of the move. Did anything change in particular or just move up/memoizing?
There was a problem hiding this comment.
Added a comment around the new bit.
| @@ -413,10 +583,12 @@ const SampleRecordCard: React.FC<SampleRecordCardProps> = ({ | |||
| ); | |||
| } | |||
|
|
|||
| if (canExportRecord) { | |||
| if (destinationId) { | |||
There was a problem hiding this comment.
As mentioned above, if the canExportRecord memo checks for destinationId, then this can go back to the way it was. Using canExportRecord over destinationId seems less arbitrary.
There was a problem hiding this comment.
Brian wanted the button always visible on the Destination page.
| const canExportRecord = useMemo( | ||
| () => destinations?.find(({ id }) => id === destinationId), | ||
| () => !!destinations?.find(({ id }) => id === destinationId), |
There was a problem hiding this comment.
Adding a short-circuit here
| <p>You need to map a property to see your sample record.</p> | ||
| </Col> | ||
| </Row> | ||
| ); |
There was a problem hiding this comment.
This bit here is new.
| @@ -366,6 +366,176 @@ const SampleRecordCard: React.FC<SampleRecordCardProps> = ({ | |||
| setExporting(false); | |||
| }, [client, destinationId, record?.id]); | |||
|
|
|||
| const content = useMemo(() => { | |||
There was a problem hiding this comment.
Added a comment around the new bit.
krishnaglick
force-pushed
the
180984244-destination-sample-record-card
branch
from
40780c4
to
e28d9d5
Compare
Change description
Destination now asks to be saved before allowing user to export.
The singular Export call is now sync.
The sample record call dynamically updates as you map/unmap properties or change groups
Improved testing a touch
Checklists
Development
Impact
Please explain any security, performance, migration, or other impacts if relevant:
Code review