You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/10/27 02:34:40 UTC
[GitHub] [incubator-superset] nytai commented on a change in pull request #11206: feat: home screen mvp
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org