[GitHub] [superset] etr2460 commented on a change in pull request #13291: fix(home): missing key and invalid dates in Recents cards

[GitBox <gi...@apache.org>]

etr2460 commented on a change in pull request #13291:
URL: https://github.com/apache/superset/pull/13291#discussion_r580725740



##########
File path: superset-frontend/src/types/Chart.ts
##########
@@ -44,4 +45,7 @@ export type Slice = {
   slice_name: string;
   description: string | null;
   cache_timeout: number | null;
+  url?: string;
 };
+
+export default Chart;

Review comment:
       Yikes, i don't even want to know why we have a `Slice` and a `Chart` type...

##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -139,35 +194,30 @@ export default function ActivityTable({
     });
   }
 
-  const renderActivity = () => {
-    const getRecentRef = (e: ActivityObjects) => {
-      if (activeChild === 'Viewed') {
-        return e.item_url;
-      }
-      return e.sql ? `/superset/sqllab?savedQueryId=${e.id}` : e.url;
-    };
-    return activityData[activeChild].map((e: ActivityObjects) => (
-      <CardStyles
-        onClick={() => {
-          window.location.href = getRecentRef(e);
-        }}
-        key={e.id}
-      >
-        <ListViewCard
-          loading={loading}
-          cover={<></>}
-          url={e.sql ? `/superset/sqllab?savedQueryId=${e.id}` : e.url}
-          title={getFilterTitle(e)}
-          description={`Last Edited: ${moment(
-            e.changed_on_utc,
-            'MM/DD/YYYY HH:mm:ss',
-          )}`}
-          avatar={getIconName(e)}
-          actions={null}
-        />
-      </CardStyles>
-    ));
-  };
+  const renderActivity = () =>
+    activityData[activeChild].map((e: ActivityObject) => {
+      const url = getEntityUrl(e);
+      const lastActionOn = getEntityLastActionOn(e);
+      return (
+        <CardStyles
+          onClick={() => {
+            window.location.href = url;
+          }}
+          key={url}

Review comment:
       is this guaranteed to be unique? The same item can't show up twice in the recent activity section?

##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -79,31 +97,68 @@ const ActivityContainer = styled.div`
   }
 `;
 
+const UNTITLED = t('[Untitled]');
+const UNKNOWN_TIME = t('Unknown');
+// translation keys for last action on
+const TRANS_LAST_VIEWED = `Last viewed %s`;
+const TRANS_LAST_MODIFIED = `Last modified %s`;

Review comment:
       does this actually work? I would've guessed you'd need to put the actual strings in the `t()` functions

##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -79,31 +97,68 @@ const ActivityContainer = styled.div`
   }
 `;
 
+const UNTITLED = t('[Untitled]');
+const UNKNOWN_TIME = t('Unknown');
+// translation keys for last action on
+const TRANS_LAST_VIEWED = `Last viewed %s`;
+const TRANS_LAST_MODIFIED = `Last modified %s`;
+
+const getEntityTitle = (e: ActivityObject) => {

Review comment:
       I know this is just a refactor, but `e` is a pretty bad variable name (i read it as `error` or `event` first, not `entity`). Can we rename throughout to `entity`?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org