suddjian commented on a change in pull request #14823:
URL: https://github.com/apache/superset/pull/14823#discussion_r650225187



##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -68,6 +75,7 @@ interface ActivityProps {
   activeChild: string;
   setActiveChild: (arg0: string) => void;
   activityData: ActivityData;
+  loadedCount: number;

Review comment:
       This should probably be a boolean `isLoaded` rather than the count, 
unless this component specifically needs the count for some reason.

##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -246,7 +254,8 @@ export default function ActivityTable({
         );
       },
     );
-  if (loadingState && !editedObjs) {
+
+  if ((loadingState && !editedObjs) || loadedCount !== 3) {

Review comment:
       I would use `<` for this check just to be safe. But actually, unless the 
number itself is important, I would probably separate the loading states into 
three different booleans and `&&` them together.

##########
File path: superset-frontend/src/views/CRUD/welcome/ChartTable.tsx
##########
@@ -59,6 +61,9 @@ function ChartTable({
   showThumbnails,
 }: ChartTableProps) {
   const history = useHistory();
+  const filterStore = getFromLocalStorage(HOMEPAGE_CHART_FILTER, null);
+  const defaultFilter = filterStore || TableTabTypes.MINE;

Review comment:
       suggestion: `initialFilter` would be a more appropriate name. "default" 
typically indicates that it will be the value used when nothing is selected, 
but in this UI the user cannot select nothing.

##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -161,10 +169,10 @@ export default function ActivityTable({
   setActiveChild,
   activityData,
   user,
+  loadedCount,
 }: ActivityProps) {
   const [editedObjs, setEditedObjs] = useState<Array<ActivityData>>();
   const [loadingState, setLoadingState] = useState(false);
-
   useEffect(() => {
     if (activeChild === 'Edited') {

Review comment:
       Should `activeChild` be in this hook's dependency list?




-- 
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:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to