nytai commented on a change in pull request #11206:
URL:
https://github.com/apache/incubator-superset/pull/11206#discussion_r512363132
##########
File path: superset-frontend/src/components/ListViewCard/index.tsx
##########
@@ -139,19 +139,39 @@ const SkeletonActions = styled(Skeleton.Button)`
width: ${({ theme }) => theme.gridUnit * 10}px;
`;
+const QueryData = styled.div`
+ display: flex;
+ flex-direction: row;
+ justify-content: flex-start;
+ border-bottom: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
+ .title {
+ font-weight: ${({ theme }) => theme.typography.weights.normal};
+ color: ${({ theme }) => theme.colors.grayscale.light2};
+ }
+ .holder {
+ margin: ${({ theme }) => theme.gridUnit * 2}px;
+ }
+`;
+
const paragraphConfig = { rows: 1, width: 150 };
interface CardProps {
title: React.ReactNode;
url?: string;
- imgURL: string;
- imgFallbackURL: string;
+ imgURL?: string;
+ tables?: string | number;
+ imgFallbackURL?: string;
imgPosition?: BackgroundPosition;
description: string;
loading: boolean;
titleRight?: React.ReactNode;
coverLeft?: React.ReactNode;
coverRight?: React.ReactNode;
- actions: React.ReactNode;
+ actions: React.ReactNode | null;
+ showImg?: boolean;
+ rows?: number | string;
+ avatar?: string;
Review comment:
nit: this should be `IconName` or `IconProps["name"]`.
##########
File path: superset-frontend/src/components/ListViewCard/index.tsx
##########
@@ -162,35 +182,54 @@ function ListViewCard({
imgFallbackURL,
description,
coverLeft,
+ tables,
coverRight,
actions,
+ avatar,
+ tableName,
loading,
imgPosition = 'top',
+ showImg = true,
+ isRecent,
}: CardProps) {
return (
<StyledCard
data-test="styled-card"
cover={
- <Cover>
- <a href={url}>
- <div className="gradient-container">
- <ImageLoader
- src={imgURL}
- fallback={imgFallbackURL}
- isLoading={loading}
- position={imgPosition}
- />
+ !isRecent &&
+ (showImg ? (
+ <Cover>
+ <a href={url}>
+ <div className="gradient-container">
+ <ImageLoader
+ src={imgURL || ''}
+ fallback={imgFallbackURL || ''}
+ isLoading={loading}
+ position={imgPosition}
+ />
+ </div>
+ </a>
+ <CoverFooter className="cover-footer">
+ {!loading && coverLeft && (
+ <CoverFooterLeft>{coverLeft}</CoverFooterLeft>
+ )}
+ {!loading && coverRight && (
+ <CoverFooterRight>{coverRight}</CoverFooterRight>
+ )}
+ </CoverFooter>
+ </Cover>
+ ) : (
+ <QueryData>
Review comment:
This seems highly coupled to the Query card implementation. Can we move
this up into the parent and instead have a `renderCover` that renders the cover
slot, or `cover` prop for this jsx.
We could also probably get rid of the `isRecent` and `showImg` props and
instead check the presence of this prop.
##########
File path: superset-frontend/src/views/CRUD/welcome/EmptyState.tsx
##########
@@ -0,0 +1,124 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import Button from 'src/components/Button';
+import { Empty } from 'src/common/components';
+import { t, styled } from '@superset-ui/core';
+import Icon from 'src/components/Icon';
+import { IconContainer } from '../utils';
+
+interface EmptyStateProps {
+ tableName: string;
+ tab?: string;
+}
+
+const ButtonContainer = styled.div`
+ Button {
+ svg {
+ color: ${({ theme }) => theme.colors.grayscale.light5};
+ }
+ }
+`;
+
+export default function EmptyState({ tableName, tab }: EmptyStateProps) {
+ const mineRedirects = {
+ DASHBOARDS: '/dashboard/new',
+ CHARTS: '/chart/add',
+ SAVED_QUERIES: '/superset/sqllab',
+ };
+ const favRedirects = {
+ DASHBOARDS: '/dashboard/list/',
+ CHARTS: '/chart/list',
+ SAVED_QUERIES: '/savedqueryview/list/',
+ };
+ const tableIcon = {
+ RECENTS: 'union.png',
+ DASHBOARDS: 'empty-dashboard.png',
+ CHARTS: 'empty-charts.png',
+ SAVED_QUERIES: 'empty-queries.png',
+ };
+ const mine = (
+ <div>{`No ${
+ tableName === 'SAVED_QUERIES'
+ ? t('saved queries')
+ : t(`${tableName.toLowerCase()}`)
+ } yet`}</div>
+ );
+ const recent = (
+ <div className="no-recents">
+ {t(
+ `Recently ${tab?.toLowerCase()} charts, dashboards, and saved queries
will appear here`,
Review comment:
I don't think translations work like this. Not all languages follow the
same grammar as english. You could use string interpolation eg `t('Recently %s
charts...', tab.toLowerCase())` so that the %s can be moved around to where it
makes the most sense but then `tab` wouldn't be translated. The correct
solution is probably to list out the variations.
##########
File path: superset-frontend/src/views/CRUD/hooks.ts
##########
@@ -348,5 +355,87 @@ export function useFavoriteStatus(
);
};
- return [favoriteStatusRef, fetchFaveStar, saveFaveStar] as const;
+ return [
+ favoriteStatusRef,
+ fetchFaveStar,
+ saveFaveStar,
+ favoriteStatus,
+ ] as const;
}
+
+export const useChartEditModal = (
+ setCharts: (charts: Array<Chart>) => void,
+ charts: Array<Chart>,
+) => {
+ const [
+ sliceCurrentlyEditing,
+ setSliceCurrentlyEditing,
+ ] = useState<Slice | null>(null);
+
+ function openChartEditModal(chart: Chart) {
+ setSliceCurrentlyEditing({
+ slice_id: chart.id,
+ slice_name: chart.slice_name,
+ description: chart.description,
+ cache_timeout: chart.cache_timeout,
+ });
+ }
+
+ function closeChartEditModal() {
+ setSliceCurrentlyEditing(null);
+ }
+
+ function handleChartUpdated(edits: Chart) {
+ // update the chart in our state with the edited info
+ const newCharts = charts.map((chart: Chart) =>
+ chart.id === edits.id ? { ...chart, ...edits } : chart,
+ );
+ setCharts(newCharts);
+ }
+
+ return {
+ sliceCurrentlyEditing,
+ handleChartUpdated,
+ openChartEditModal,
+ closeChartEditModal,
+ };
+};
+
+export const copyQueryLink = (
Review comment:
π we should only have 1 copy to clipboard util function
##########
File path: superset-frontend/src/components/ListViewCard/index.tsx
##########
@@ -162,35 +182,54 @@ function ListViewCard({
imgFallbackURL,
description,
coverLeft,
+ tables,
coverRight,
actions,
+ avatar,
+ tableName,
loading,
imgPosition = 'top',
+ showImg = true,
+ isRecent,
Review comment:
Is this prop doing anything else besides showing/hiding the cover image
(along with the showImg prop)?
##########
File path: superset-frontend/src/views/CRUD/hooks.ts
##########
@@ -348,5 +349,87 @@ export function useFavoriteStatus(
);
};
- return [favoriteStatusRef, fetchFaveStar, saveFaveStar] as const;
+ return [
+ favoriteStatusRef,
+ fetchFaveStar,
+ saveFaveStar,
+ favoriteStatus,
Review comment:
There's an issue with exposing this state, due to closures some
functions end up with a stale ref to this state. We expose the
favoriteStatusRef for this reason, it's always pointing to the latest version
of favoriteStatus object. Is there a reason we're exposing this instead of
using `favoriteStatusRef.current` as we do elsewhere?
##########
File path: superset-frontend/src/components/ListViewCard/index.tsx
##########
@@ -162,35 +182,54 @@ function ListViewCard({
imgFallbackURL,
description,
coverLeft,
+ tables,
coverRight,
actions,
+ avatar,
+ tableName,
loading,
imgPosition = 'top',
+ showImg = true,
+ isRecent,
}: CardProps) {
return (
<StyledCard
data-test="styled-card"
cover={
- <Cover>
- <a href={url}>
- <div className="gradient-container">
- <ImageLoader
- src={imgURL}
- fallback={imgFallbackURL}
- isLoading={loading}
- position={imgPosition}
- />
+ !isRecent &&
+ (showImg ? (
+ <Cover>
+ <a href={url}>
+ <div className="gradient-container">
+ <ImageLoader
+ src={imgURL || ''}
+ fallback={imgFallbackURL || ''}
+ isLoading={loading}
+ position={imgPosition}
+ />
+ </div>
+ </a>
+ <CoverFooter className="cover-footer">
+ {!loading && coverLeft && (
+ <CoverFooterLeft>{coverLeft}</CoverFooterLeft>
+ )}
+ {!loading && coverRight && (
+ <CoverFooterRight>{coverRight}</CoverFooterRight>
+ )}
+ </CoverFooter>
+ </Cover>
+ ) : (
+ <QueryData>
+ <div className="holder">
+ <div className="title">Tables</div>
Review comment:
```suggestion
<div className="title">{t('Tables')}</div>
```
##########
File path: superset-frontend/src/views/CRUD/dashboard/DashboardList.tsx
##########
@@ -418,83 +417,18 @@ function DashboardList(props: DashboardListProps) {
},
];
- function renderCard(dashboard: Dashboard & { loading: boolean }) {
- const menu = (
- <Menu>
- {canDelete && (
- <Menu.Item>
- <ConfirmStatusChange
- title={t('Please Confirm')}
- description={
- <>
- {t('Are you sure you want to delete')}{' '}
- <b>{dashboard.dashboard_title}</b>?
- </>
- }
- onConfirm={() => handleDashboardDelete(dashboard)}
- >
- {confirmDelete => (
- <div
- role="button"
- tabIndex={0}
- className="action-button"
- onClick={confirmDelete}
- >
- <ListViewCard.MenuIcon
- data-test="dashboard-list-view-card-trash-icon"
- name="trash"
- />{' '}
- Delete
- </div>
- )}
- </ConfirmStatusChange>
- </Menu.Item>
- )}
- {canExport && (
- <Menu.Item
- role="button"
- tabIndex={0}
- onClick={() => handleBulkDashboardExport([dashboard])}
- >
- <ListViewCard.MenuIcon name="share" /> Export
- </Menu.Item>
- )}
- {canEdit && (
- <Menu.Item
- data-test="dashboard-list-edit-option"
- role="button"
- tabIndex={0}
- onClick={() => openDashboardEditModal(dashboard)}
- >
- <ListViewCard.MenuIcon name="edit-alt" /> Edit
- </Menu.Item>
- )}
- </Menu>
- );
-
+ function renderCard(dashboard: Dashboard) {
return (
- <ListViewCard
- loading={dashboard.loading}
- title={dashboard.dashboard_title}
- titleRight={
- <Label>{dashboard.published ? 'published' : 'draft'}</Label>
- }
- url={bulkSelectEnabled ? undefined : dashboard.url}
- imgURL={dashboard.thumbnail_url}
- imgFallbackURL="/static/assets/images/dashboard-card-fallback.png"
- description={t(
- 'Last modified %s',
- dashboard.changed_on_delta_humanized,
- )}
- coverLeft={<FacePile users={dashboard.owners || []} />}
- actions={
- <ListViewCard.Actions>
- {renderFaveStar(dashboard.id)}
- <Dropdown overlay={menu}>
- <Icon name="more-horiz" />
- </Dropdown>
- </ListViewCard.Actions>
- }
+ <DashboardCard
+ {...{
Review comment:
nit: is there a reason to spread these instead of just listing out the
props?
##########
File path: superset-frontend/src/components/ListViewCard/index.tsx
##########
@@ -162,35 +182,54 @@ function ListViewCard({
imgFallbackURL,
description,
coverLeft,
+ tables,
coverRight,
actions,
+ avatar,
+ tableName,
loading,
imgPosition = 'top',
+ showImg = true,
+ isRecent,
}: CardProps) {
return (
<StyledCard
data-test="styled-card"
cover={
- <Cover>
- <a href={url}>
- <div className="gradient-container">
- <ImageLoader
- src={imgURL}
- fallback={imgFallbackURL}
- isLoading={loading}
- position={imgPosition}
- />
+ !isRecent &&
+ (showImg ? (
+ <Cover>
+ <a href={url}>
+ <div className="gradient-container">
+ <ImageLoader
+ src={imgURL || ''}
+ fallback={imgFallbackURL || ''}
+ isLoading={loading}
+ position={imgPosition}
+ />
+ </div>
+ </a>
+ <CoverFooter className="cover-footer">
+ {!loading && coverLeft && (
+ <CoverFooterLeft>{coverLeft}</CoverFooterLeft>
+ )}
+ {!loading && coverRight && (
+ <CoverFooterRight>{coverRight}</CoverFooterRight>
+ )}
+ </CoverFooter>
+ </Cover>
+ ) : (
+ <QueryData>
+ <div className="holder">
+ <div className="title">Tables</div>
+ <div>{tables}</div>
+ </div>
+ <div className="holder">
+ <div className="title">Datasource Name</div>
Review comment:
```suggestion
<div className="title">{t('Datasource Name')}</div>
```
##########
File path: superset-frontend/src/views/CRUD/dashboard/DashboardCard.tsx
##########
@@ -0,0 +1,159 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { SupersetClient, t } from '@superset-ui/core';
+import rison from 'rison';
+import { Dropdown, Menu } from 'src/common/components';
+import ConfirmStatusChange from 'src/components/ConfirmStatusChange';
+import ListViewCard from 'src/components/ListViewCard';
+import Icon from 'src/components/Icon';
+import Label from 'src/components/Label';
+import FacePile from 'src/components/FacePile';
+import FaveStar from 'src/components/FaveStar';
+import { DashboardCardProps, Dashboard } from 'src/views/CRUD/types';
+import { createErrorHandler } from 'src/views/CRUD/utils';
+import { useFavoriteStatus } from 'src/views/CRUD/hooks';
+
+const FAVESTAR_BASE_URL = '/superset/favstar/Dashboard';
+
+function DashboardCard({
+ isChart,
Review comment:
What does this prop do? It doesn't look like it's actually being passed
in anywhere
##########
File path: superset-frontend/src/views/CRUD/utils.tsx
##########
@@ -53,6 +54,98 @@ const createFetchResourceMethod = (method: string) => (
return [];
};
+export const getRecentAcitivtyObjs = (
+ userId: string | number,
+ recent: string,
+) => {
+ const getParams = (filters?: Array<any>) => {
+ const params = {
+ order_column: 'changed_on_delta_humanized',
Review comment:
should this be `changed_on`?
##########
File path: superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
##########
@@ -0,0 +1,209 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useState } from 'react';
+import moment from 'antd/node_modules/moment';
+import { styled, t } from '@superset-ui/core';
+
+import ListViewCard from 'src/components/ListViewCard';
+import { addDangerToast } from 'src/messageToasts/actions';
+import SubMenu from 'src/components/Menu/SubMenu';
+import { reject } from 'lodash';
+import { getRecentAcitivtyObjs, mq } from '../utils';
+import EmptyState from './EmptyState';
+
+interface MapProps {
Review comment:
nit: `ActivityProps` or `ActivityObjectProps` seems more descriptive. My
mind went to a map πΊοΈ when I saw this.
##########
File path: superset-frontend/src/views/CRUD/chart/ChartCard.tsx
##########
@@ -0,0 +1,141 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import { useFavoriteStatus } from 'src/views/CRUD/hooks';
+import { SupersetClient, t } from '@superset-ui/core';
+import ConfirmStatusChange from 'src/components/ConfirmStatusChange';
+import Icon from 'src/components/Icon';
+import Chart from 'src/types/Chart';
+
+import ListViewCard from 'src/components/ListViewCard';
+import Label from 'src/components/Label';
+import { Dropdown, Menu } from 'src/common/components';
+import FaveStar from 'src/components/FaveStar';
+import FacePile from 'src/components/FacePile';
+
+const FAVESTAR_BASE_URL = '/superset/favstar/slice';
+
+interface ChartCardProps {
+ chart: Chart;
+ hasPerm: (perm: string) => boolean;
+ openChartEditModal: (chart: Chart) => void;
+ bulkSelectEnabled: boolean;
+ addDangerToast: (msg: string) => void;
+ addSuccessToast: (msg: string) => void;
+ refreshData: () => void;
+ loading: boolean;
+}
+
+export default function ChartCard({
+ chart,
+ hasPerm,
+ openChartEditModal,
+ bulkSelectEnabled,
+ addDangerToast,
+ addSuccessToast,
+ refreshData,
+ loading,
+}: ChartCardProps) {
+ const canEdit = hasPerm('can_edit');
+ const canDelete = hasPerm('can_delete');
+ const [, fetchFaveStar, saveFaveStar, favoriteStatus] = useFavoriteStatus(
+ {},
+ FAVESTAR_BASE_URL,
+ addDangerToast,
+ );
+ function handleChartDelete({ id, slice_name: sliceName }: Chart) {
+ SupersetClient.delete({
+ endpoint: `/api/v1/chart/${id}`,
+ }).then(
+ () => {
+ refreshData();
+ addSuccessToast(t('Deleted: %s', sliceName));
+ },
+ () => {
+ addDangerToast(t('There was an issue deleting: %s', sliceName));
+ },
+ );
+ }
Review comment:
I'm not sure it makes the most sense to move all this logic down into
these card components. It does make these components more standalone, however
the logic will be duplicated in the case of the DashboardList and ChartList
components. There will be 2 instances of the `useFaveStar` hook mounted. Also
why is `openChartEditModal` passed in as a prop, yet `handleChartDelete` is
implemented here?
If we're trying to dry up these actions between the home screen and the
(Chart|Dashboard)List we could probably achieve this by moving them into a
shared utils module, eg.
`export const createChartDeleteFunction = (handleSuccess, handleError) =>
({id, slice_name....`
----------------------------------------------------------------
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]
