[GitHub] [superset] samtfm commented on a diff in pull request #21033: feat: add extension point for workspace home page

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

samtfm commented on code in PR #21033:
URL: https://github.com/apache/superset/pull/21033#discussion_r942717817


##########
superset-frontend/src/views/CRUD/welcome/Welcome.tsx:
##########
@@ -179,7 +179,10 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
     setItem(LocalStorageKeys.homepage_collapse_state, state);
   };
 
+  const WelcomeMessageExtension = extensionsRegistry.get('welcome.message');
   const WelcomeTopExtension = extensionsRegistry.get('welcome.banner');
+  const WelcomeDataExtension = extensionsRegistry.get('welcome.data');
+  const WelcomeTableExtension = extensionsRegistry.get('welcome.table');

Review Comment:
   I think the convention for these names is they should refer mostly to the place in the layout that the extension will go, without referring to the content that Preset will be putting into the extension. 
   
   Ideally, these new components could fit under a single extension point, maybe something like "welcome.main.replacement" since it's replacing the body content of the welcome page.  The details that Preset uses this extension point to add extra data and a table isn't really relevant to the superset side.



##########
superset-frontend/src/views/CRUD/welcome/Welcome.tsx:
##########
@@ -282,71 +285,87 @@ function Welcome({ user, addDangerToast }: WelcomeProps) {
     !activityData?.Examples && !activityData?.Viewed;
   return (
     <WelcomeContainer>
+      {WelcomeMessageExtension && <WelcomeMessageExtension user={user} />}
       {WelcomeTopExtension && <WelcomeTopExtension />}
-      <WelcomeNav>
-        <h1 className="welcome-header">Home</h1>
-        {isFeatureEnabled(FeatureFlag.THUMBNAILS) ? (
-          <div className="switch">
-            <AntdSwitch checked={checked} onChange={handleToggle} />
-            <span>Thumbnails</span>
-          </div>
-        ) : null}
-      </WelcomeNav>
-      <Collapse activeKey={activeState} onChange={handleCollapse} ghost bigger>
-        <Collapse.Panel header={t('Recents')} key="1">
-          {activityData &&
-          (activityData.Viewed ||
-            activityData.Examples ||
-            activityData.Created) &&
-          activeChild !== 'Loading' ? (
-            <ActivityTable
-              user={{ userId: user.userId! }} // user is definitely not a guest user on this page
-              activeChild={activeChild}
-              setActiveChild={setActiveChild}
-              activityData={activityData}
-              loadedCount={loadedCount}
-            />
-          ) : (
-            <LoadingCards />
-          )}
-        </Collapse.Panel>
-        <Collapse.Panel header={t('Dashboards')} key="2">
-          {!dashboardData || isRecentActivityLoading ? (
-            <LoadingCards cover={checked} />
-          ) : (
-            <DashboardTable
-              user={user}
-              mine={dashboardData}
-              showThumbnails={checked}
-              examples={activityData?.Examples}
-            />
-          )}
-        </Collapse.Panel>
-        <Collapse.Panel header={t('Charts')} key="3">
-          {!chartData || isRecentActivityLoading ? (
-            <LoadingCards cover={checked} />
-          ) : (
-            <ChartTable
-              showThumbnails={checked}
-              user={user}
-              mine={chartData}
-              examples={activityData?.Examples}
-            />
-          )}
-        </Collapse.Panel>
-        <Collapse.Panel header={t('Saved queries')} key="4">
-          {!queryData ? (
-            <LoadingCards cover={checked} />
-          ) : (
-            <SavedQueries
-              showThumbnails={checked}
-              user={user}
-              mine={queryData}
-              featureFlag={isFeatureEnabled(FeatureFlag.THUMBNAILS)}
-            />
-          )}
-        </Collapse.Panel>
-      </Collapse>
+      {WelcomeDataExtension && (
+        <WelcomeDataExtension examples={activityData?.Examples} user={user} />
+      )}
+      {WelcomeTableExtension && <WelcomeTableExtension user={user} />}

Review Comment:
   If possible, I think it would be best to avoid passing props to extensions to leave them more general purpose. 
   
   That said, having extra unused props shouldn't really cause issues in someone else's extension, so some amount of this pattern is probably fine if there isn't a clean way to get this data from hooks inside the component.



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

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