Feature/CV2-5785: media dialog view ux updates to include media match reason #2256

danielevalverde · 2025-01-21T00:15:58Z

Description

Add the new component to the required sections of the MediaPage.
Update GraphQL query to fetch the newly added media_cluster_origin fields.
Update unit tests
Update follow-up ticket for the refactoring:

Type of change

Performance improvement and/or refactoring (non-breaking change that keeps existing functionality)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Security mitigation or enhancement
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Automated test (add or update automated tests)

How has this been tested?

manually and running the unit tests

Things to pay attention to during code review

Please review the strings in the formatted messages
We have a separate ticket to handle the Graphql refactoring: CV2-6006

Checklist

brianfleming

looking good @danielevalverde
i just made some small tweaks mostly about being explicit and using "cluster of media"
instead of just cluster in some places

brianfleming · 2025-01-21T14:50:41Z

src/app/components/media/MediaOrigin.js

@@ -42,10 +42,10 @@ const getIconAndMessage = (type, user) => {
      ),
      tooltipMessage: (
        <FormattedHTMLMessage
-          defaultMessage="<strong>{user}</strong> uploaded this media using the Check interface"
+          defaultMessage="<strong>{media_cluster_origin_user}</strong> uploaded this media using the Check interface"


I would change slightly from:
...using the Check interface
to
...using Check

brianfleming · 2025-01-21T14:51:11Z

src/app/components/media/MediaOrigin.js

@@ -61,10 +61,10 @@ const getIconAndMessage = (type, user) => {
      ),
      tooltipMessage: (
        <FormattedHTMLMessage
-          defaultMessage="<strong>{user}</strong> added this media by merging from another cluster"
+          defaultMessage="<strong>{media_cluster_origin_user}</strong> added this media by merging from another cluster"


please change from
...merging from another cluster
to
...merging from another cluster of media

brianfleming · 2025-01-21T14:52:30Z

src/app/components/media/MediaOriginBanner.js

@@ -51,10 +53,10 @@ const getIconAndMessage = (type, user, cluster, timestamp) => {
      message: (
        <>
          <FormattedHTMLMessage
-            defaultMessage="This media was added to the cluster by <strong>{user}</strong> when merged from <strong><u>{cluster}</u></strong> "
-            description="Message for User Matched"
+            defaultMessage="This media was merged to the cluster by <strong>{media_cluster_origin_user}</strong>, "


Please change from
This media was merged to the cluster by...
to
This media was merged into this cluster of media by...

brianfleming · 2025-01-21T14:52:56Z

src/app/components/media/MediaOriginBanner.js

@@ -66,10 +68,10 @@ const getIconAndMessage = (type, user, cluster, timestamp) => {
      message: (
        <>
          <FormattedHTMLMessage
-            defaultMessage="This media was added to the cluster by <strong>{user}</strong> when accepted from <strong><u>{cluster}</u></strong> "
+            defaultMessage="This media was added to the cluster by <strong>{media_cluster_origin_confirmed_by}</strong> when accepted from <strong><u>{media_cluster_origin_title}</u></strong>, "


from
This media was added to the cluster by...
to
This media was added to the cluster of media by...

brianfleming · 2025-01-21T14:53:27Z

src/app/components/media/MediaOriginBanner.js

@@ -81,7 +83,7 @@ const getIconAndMessage = (type, user, cluster, timestamp) => {
      message: (
        <>
          <FormattedHTMLMessage
-            defaultMessage="This media was automatically matched to the cluster "
+            defaultMessage="This media was automatically matched to the cluster, "


from
...matched to the cluster,...
to
...matched to the cluster of media,...

danielevalverde · 2025-01-21T19:43:43Z

looking good @danielevalverde i just made some small tweaks mostly about being explicit and using "cluster of media" instead of just cluster in some places

addressed the review comments 🫡

amoedoamorim · 2025-01-22T17:38:38Z

src/app/components/media/MediaComponent.js

@@ -214,6 +215,14 @@ class MediaComponent extends Component {
                        )]}
                      />
                    }
+                    media_origin_banner={


prop names should follow camelCase => mediaOriginBanner={

amoedoamorim · 2025-01-22T17:39:27Z

src/app/components/media/MediaOriginBanner.js

@@ -115,6 +115,10 @@ const MediaOriginBanner = ({
  );
 };

+MediaOriginBanner.defaultProps = {
+  media_cluster_relationship: {},


prop names should follow camelCase

amoedoamorim

Please change props that are passed with snake_case to camelCase

brianfleming

nice, thanks for that data fix @danielevalverde !

just one small UI nit to fix on my end:

please add maybe 8px of space below the banner in the dialog, so its not right up against the media like in the screenshot here:

danielevalverde added 5 commits January 20, 2025 09:16

adding MediaOriginBanner component to media page

4afa1b8

passing media_clust_relantioship props

1823f14

refactoring and renaming props

1227945

more code cleanup

ea3b53f

remove component from sandbox

8954e9d

danielevalverde marked this pull request as ready for review January 21, 2025 00:27

danielevalverde requested review from amoedoamorim, brianfleming and sarahkeh as code owners January 21, 2025 00:27

danielevalverde added 2 commits January 20, 2025 21:29

updating unit tests

567950f

Fix linter problems

0053975

brianfleming requested changes Jan 21, 2025

View reviewed changes

danielevalverde added 4 commits January 21, 2025 14:53

apply PR review

da5c700

remove more debugs

a579be2

apply PR programing with Alex

582c346

update tests

ad7b79d

danielevalverde requested a review from brianfleming January 21, 2025 19:42

add MediaOriginBanner to MediaComponent

47bb428

amoedoamorim reviewed Jan 22, 2025

View reviewed changes

amoedoamorim requested changes Jan 22, 2025

View reviewed changes

brianfleming requested changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/CV2-5785: media dialog view ux updates to include media match reason #2256

Feature/CV2-5785: media dialog view ux updates to include media match reason #2256

danielevalverde commented Jan 21, 2025 •

edited

Loading

brianfleming left a comment

brianfleming Jan 21, 2025

danielevalverde Jan 21, 2025

brianfleming Jan 21, 2025

danielevalverde Jan 21, 2025

brianfleming Jan 21, 2025

danielevalverde Jan 21, 2025

brianfleming Jan 21, 2025

danielevalverde Jan 21, 2025

brianfleming Jan 21, 2025

danielevalverde Jan 21, 2025

danielevalverde commented Jan 21, 2025

amoedoamorim Jan 22, 2025

amoedoamorim Jan 22, 2025

amoedoamorim left a comment

brianfleming left a comment

Feature/CV2-5785: media dialog view ux updates to include media match reason #2256

Are you sure you want to change the base?

Feature/CV2-5785: media dialog view ux updates to include media match reason #2256

Conversation

danielevalverde commented Jan 21, 2025 • edited Loading

Description

Type of change

How has this been tested?

Things to pay attention to during code review

Checklist

brianfleming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielevalverde commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amoedoamorim left a comment

Choose a reason for hiding this comment

brianfleming left a comment

Choose a reason for hiding this comment

danielevalverde commented Jan 21, 2025 •

edited

Loading