ktmud commented on a change in pull request #11206: URL: https://github.com/apache/incubator-superset/pull/11206#discussion_r514000670
########## File path: superset-frontend/spec/javascripts/views/CRUD/welcome/ActivityTable_spec.tsx ########## @@ -0,0 +1,87 @@ +/** + * 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 { styledMount as mount } from 'spec/helpers/theming'; +import thunk from 'redux-thunk'; +import fetchMock from 'fetch-mock'; + +import waitForComponentToPaint from 'spec/helpers/waitForComponentToPaint'; +import configureStore from 'redux-mock-store'; +import ActivityTable from 'src/views/CRUD/welcome/ActivityTable'; + +const mockStore = configureStore([thunk]); +const store = mockStore({}); + +const chartsEndpoint = 'glob:*/api/v1/chart/?*'; +const dashboardEndpoint = 'glob:*/api/v1/dashboard/?*'; +const savedQueryEndpoint = 'glob:*/api/v1/saved_query/?*'; + +fetchMock.get(chartsEndpoint, { + result: [ + { + slice_name: 'ChartyChart', + changed_on_utc: '24 Feb 2014 10:13:14', + url: '/fakeUrl/explore', + id: '4', + table: {}, + }, + ], +}); + +fetchMock.get(dashboardEndpoint, { + result: [ + { + dashboard_title: 'Dashboard_Test', + changed_on_utc: '24 Feb 2014 10:13:14', + url: '/fakeUrl/dashboard', + id: '3', + }, + ], +}); + +fetchMock.get(savedQueryEndpoint, { + result: [], +}); + +describe('ActivityTable', () => { + const activityProps = { + user: { + userId: '1', + }, + activityFilter: 'Edited', + }; + const wrapper = mount(<ActivityTable {...activityProps} />, { + context: { store }, + }); + + beforeAll(async () => { + await waitForComponentToPaint(wrapper); + }); + + it('the component renders ', () => { + expect(wrapper.find(ActivityTable)).toExist(); + }); + + it('calls batch method and renders ListViewCArd', async () => { + const chartCall = fetchMock.calls(/chart\/\?q/); + const dashboardCall = fetchMock.calls(/dashboard\/\?q/); + expect(chartCall).toHaveLength(2); + expect(dashboardCall).toHaveLength(2); Review comment: The home page fires 12 fetch requests on initial page load: <img src="https://user-images.githubusercontent.com/335541/97531220-f907c680-1970-11eb-9929-8f83130092e8.png"; width="500"> This would be quite unscalable for large Superset deployments. Can we combine some of these or make them async? For example, the non-default tabs shouldn't render when they are not selected. ########## File path: superset-frontend/src/views/CRUD/welcome/EmptyState.tsx ########## @@ -0,0 +1,144 @@ +/** + * 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', Review comment: The images are quite blurry on retina screens. Can we use SVG icons or at least 2x images? <img src="https://user-images.githubusercontent.com/335541/97531553-a4b11680-1971-11eb-9f6f-5ca502973be2.png"; width="400"> ########## File path: superset-frontend/src/views/CRUD/welcome/DashboardTable.tsx ########## @@ -16,169 +16,176 @@ * specific language governing permissions and limitations * under the License. */ -import React from 'react'; -import { t, SupersetClient } from '@superset-ui/core'; -import { debounce } from 'lodash'; -import ListView, { FetchDataConfig } from 'src/components/ListView'; +import React, { useEffect, useState } from 'react'; +import { SupersetClient, t } from '@superset-ui/core'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { Dashboard, DashboardTableProps } from 'src/views/CRUD/types'; import withToasts from 'src/messageToasts/enhancers/withToasts'; -import { Dashboard } from 'src/types/bootstrapTypes'; +import PropertiesModal from 'src/dashboard/components/PropertiesModal'; +import DashboardCard from 'src/views/CRUD/dashboard/DashboardCard'; +import SubMenu from 'src/components/Menu/SubMenu'; +import Icon from 'src/components/Icon'; +import EmptyState from './EmptyState'; +import { createErrorHandler, CardContainer, IconContainer } from '../utils'; -const PAGE_SIZE = 25; +const PAGE_SIZE = 3; -interface DashboardTableProps { - addDangerToast: (message: string) => void; - search?: string; +export interface FilterValue { + col: string; + operator: string; + value: string | boolean | number | null | undefined; } -interface DashboardTableState { - dashboards: Dashboard[]; - dashboard_count: number; - loading: boolean; -} +function DashboardTable({ + user, + addDangerToast, + addSuccessToast, +}: DashboardTableProps) { + const { + state: { loading, resourceCollection: dashboards, bulkSelectEnabled }, + setResourceCollection: setDashboards, + hasPerm, + refreshData, + fetchData, + } = useListViewResource<Dashboard>( + 'dashboard', + t('dashboard'), + addDangerToast, + ); -class DashboardTable extends React.PureComponent< - DashboardTableProps, - DashboardTableState -> { - columns = [ - { - accessor: 'dashboard_title', - Header: 'Dashboard', - Cell: ({ - row: { - original: { url, dashboard_title: dashboardTitle }, - }, - }: { - row: { - original: { - url: string; - dashboard_title: string; - }; - }; - }) => <a href={url}>{dashboardTitle}</a>, - }, - { - accessor: 'changed_by.first_name', - Header: 'Modified By', - Cell: ({ - row: { - original: { changed_by_name: changedByName, changedByUrl }, - }, - }: { - row: { - original: { - changed_by_name: string; - changedByUrl: string; - }; - }; - }) => <a href={changedByUrl}>{changedByName}</a>, - }, - { - accessor: 'changed_on_delta_humanized', - Header: 'Modified', - Cell: ({ - row: { - original: { changed_on_delta_humanized: changedOn }, - }, - }: { - row: { - original: { - changed_on_delta_humanized: string; - }; - }; - }) => <span className="no-wrap">{changedOn}</span>, - }, - ]; + const [editModal, setEditModal] = useState<Dashboard | null>(null); + const [dashboardFilter, setDashboardFilter] = useState('Favorite'); - initialSort = [{ id: 'changed_on_delta_humanized', desc: true }]; + const handleDashboardEdit = (edits: Dashboard) => { + return SupersetClient.get({ + endpoint: `/api/v1/dashboard/${edits.id}`, + }).then( + ({ json = {} }) => { + setDashboards( + dashboards.map(dashboard => { + if (dashboard.id === json.id) { + return json.result; + } + return dashboard; + }), + ); + }, + createErrorHandler(errMsg => + addDangerToast( + t('An error occurred while fetching dashboards: %s', errMsg), + ), + ), + ); + }; - constructor(props: DashboardTableProps) { - super(props); - this.state = { - dashboards: [], - dashboard_count: 0, - loading: false, - }; - } + const getFilters = () => { + const filters = []; - componentDidUpdate(prevProps: DashboardTableProps) { - if (prevProps.search !== this.props.search) { - this.fetchDataDebounced({ - pageSize: PAGE_SIZE, - pageIndex: 0, - sortBy: this.initialSort, - filters: [], + if (dashboardFilter === 'Mine') { + filters.push({ + id: 'owners', + operator: 'rel_m_m', + value: `${user?.userId}`, + }); + } else { + filters.push({ + id: 'id', + operator: 'dashboard_is_fav', + value: true, }); } + return filters; + }; + const subMenus = []; + if (dashboards.length > 0 && dashboardFilter === 'favorite') { + subMenus.push({ + name: 'Favorite', + label: t('Favorite'), + onClick: () => setDashboardFilter('Favorite'), + }); } - fetchData = ({ pageIndex, pageSize, sortBy, filters }: FetchDataConfig) => { - this.setState({ loading: true }); - const filterExps = Object.keys(filters) - .map(fk => ({ - col: fk, - opr: filters[fk].filterId, - value: filters[fk].filterValue, - })) - .concat( - this.props.search - ? [ - { - col: 'dashboard_title', - opr: 'ct', - value: this.props.search, - }, - ] - : [], - ); - - const queryParams = JSON.stringify({ - order_column: sortBy[0].id, - order_direction: sortBy[0].desc ? 'desc' : 'asc', - page: pageIndex, - page_size: pageSize, - ...(filterExps.length ? { filters: filterExps } : {}), + useEffect(() => { + fetchData({ + pageIndex: 0, + pageSize: PAGE_SIZE, + sortBy: [ + { + id: 'changed_on_delta_humanized', + desc: true, + }, + ], + filters: getFilters(), }); + }, [dashboardFilter]); - return SupersetClient.get({ - endpoint: `/api/v1/dashboard/?q=${queryParams}`, - }) - .then(({ json }) => { - this.setState({ dashboards: json.result, dashboard_count: json.count }); - }) - .catch(response => { - if (response.status === 401) { - this.props.addDangerToast( - t( - "You don't have the necessary permissions to load dashboards. Please contact your administrator.", + return ( + <> + <SubMenu + activeChild={dashboardFilter} + tabs={[ + { + name: 'Favorite', + label: t('Favorite'), + onClick: () => setDashboardFilter('Favorite'), + }, + { + name: 'Mine', + label: t('Mine'), + onClick: () => setDashboardFilter('Mine'), + }, + ]} + buttons={[ + { + name: ( + <IconContainer> + <Icon name="plus-small" /> Dashboard{' '} + </IconContainer> ), - ); - } else { - this.props.addDangerToast( - t('An error occurred while fetching Dashboards'), - ); - } - }) - .finally(() => this.setState({ loading: false })); - }; - - // sort-comp disabled because of conflict with no-use-before-define rule - // eslint-disable-next-line react/sort-comp - fetchDataDebounced = debounce(this.fetchData, 200); - - render() { - return ( - <ListView - columns={this.columns} - data={this.state.dashboards} - count={this.state.dashboard_count} - pageSize={PAGE_SIZE} - fetchData={this.fetchData} - loading={this.state.loading} - initialSort={this.initialSort} + buttonStyle: 'tertiary', + onClick: () => { + window.location.href = '/dashboard/new'; + }, + }, + { + name: 'View All »', + buttonStyle: 'link', + onClick: () => { + window.location.href = '/dashboard/list/'; + }, + }, + ]} /> - ); - } + {editModal && ( + <PropertiesModal + dashboardId={editModal?.id} + show + onHide={() => setEditModal(null)} Review comment: ```suggestion onHide={() => setEditModal(undefined)} ``` ########## File path: superset-frontend/spec/javascripts/views/CRUD/welcome/EmptyState_spec.tsx ########## @@ -0,0 +1,92 @@ +/** + * 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 { styledMount as mount } from 'spec/helpers/theming'; +import EmptyState from 'src/views/CRUD/welcome/EmptyState'; + +describe('EmptyState', () => { + const variants = [ + { + tab: 'Favorite', + tableName: 'DASHBOARDS', + }, + { + tab: 'Mine', + tableName: 'DASHBOARDS', + }, + { + tab: 'Favorite', + tableName: 'CHARTS', + }, + { + tab: 'Mine', + tableName: 'CHARTS', + }, + { + tab: 'Favorite', + tableName: 'SAVED_QUERIES', + }, + { + tab: 'Mine', + tableName: 'SAVED_QUEREIS', + }, + ]; + const recents = [ + { + tab: 'Viewed', + tableName: 'RECENTS', + }, + { + tab: 'Edited', + tableName: 'RECENTS', + }, + { + tab: 'Created', + tableName: 'RECENTS', + }, + ]; Review comment: It probably makes more sense to make "Created" and "Mine" as the default tabs (or remember users' last selections?). <img src="https://user-images.githubusercontent.com/335541/97532128-eb534080-1972-11eb-807e-a6e2e87f1213.png"; width="500"> ########## File path: superset-frontend/src/views/CRUD/welcome/DashboardTable.tsx ########## @@ -16,169 +16,176 @@ * specific language governing permissions and limitations * under the License. */ -import React from 'react'; -import { t, SupersetClient } from '@superset-ui/core'; -import { debounce } from 'lodash'; -import ListView, { FetchDataConfig } from 'src/components/ListView'; +import React, { useEffect, useState } from 'react'; +import { SupersetClient, t } from '@superset-ui/core'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { Dashboard, DashboardTableProps } from 'src/views/CRUD/types'; import withToasts from 'src/messageToasts/enhancers/withToasts'; -import { Dashboard } from 'src/types/bootstrapTypes'; +import PropertiesModal from 'src/dashboard/components/PropertiesModal'; +import DashboardCard from 'src/views/CRUD/dashboard/DashboardCard'; +import SubMenu from 'src/components/Menu/SubMenu'; +import Icon from 'src/components/Icon'; +import EmptyState from './EmptyState'; +import { createErrorHandler, CardContainer, IconContainer } from '../utils'; -const PAGE_SIZE = 25; +const PAGE_SIZE = 3; -interface DashboardTableProps { - addDangerToast: (message: string) => void; - search?: string; +export interface FilterValue { + col: string; + operator: string; + value: string | boolean | number | null | undefined; } -interface DashboardTableState { - dashboards: Dashboard[]; - dashboard_count: number; - loading: boolean; -} +function DashboardTable({ + user, + addDangerToast, + addSuccessToast, +}: DashboardTableProps) { + const { + state: { loading, resourceCollection: dashboards, bulkSelectEnabled }, + setResourceCollection: setDashboards, + hasPerm, + refreshData, + fetchData, + } = useListViewResource<Dashboard>( + 'dashboard', + t('dashboard'), + addDangerToast, + ); -class DashboardTable extends React.PureComponent< - DashboardTableProps, - DashboardTableState -> { - columns = [ - { - accessor: 'dashboard_title', - Header: 'Dashboard', - Cell: ({ - row: { - original: { url, dashboard_title: dashboardTitle }, - }, - }: { - row: { - original: { - url: string; - dashboard_title: string; - }; - }; - }) => <a href={url}>{dashboardTitle}</a>, - }, - { - accessor: 'changed_by.first_name', - Header: 'Modified By', - Cell: ({ - row: { - original: { changed_by_name: changedByName, changedByUrl }, - }, - }: { - row: { - original: { - changed_by_name: string; - changedByUrl: string; - }; - }; - }) => <a href={changedByUrl}>{changedByName}</a>, - }, - { - accessor: 'changed_on_delta_humanized', - Header: 'Modified', - Cell: ({ - row: { - original: { changed_on_delta_humanized: changedOn }, - }, - }: { - row: { - original: { - changed_on_delta_humanized: string; - }; - }; - }) => <span className="no-wrap">{changedOn}</span>, - }, - ]; + const [editModal, setEditModal] = useState<Dashboard | null>(null); Review comment: ```suggestion const [editModal, setEditModal] = useState<Dashboard>(); ``` nit: you can skip `null` by using `undefined`. ########## File path: superset-frontend/src/components/Menu/SubMenu.tsx ########## @@ -83,8 +97,8 @@ export interface ButtonProps { export interface SubMenuProps { buttons?: Array<ButtonProps>; - name: string; - children?: MenuChild[]; + name?: string; + tabs?: MenuChild[]; Review comment: Why change it from `children` to `tabs`? Passing props kind of goes against React's declarative nature. ########## 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 ActivityObjects { + action?: string; + item_title?: string; + slice_name: string; + time: string; + changed_on_utc: string; + url: string; + sql: string; + dashboard_title: string; + label: string; + id: string; + table: object; + item_url: string; +} + +interface ActivityProps { + user: { + userId: string | number; + }; +} + +interface ActivityData { + Created?: Array<object>; + Edited?: Array<object>; + Viewed?: Array<object>; + Examples?: Array<object>; +} + +const ActivityContainer = styled.div` + margin-left: ${({ theme }) => theme.gridUnit * 2}px; + margin-top: ${({ theme }) => theme.gridUnit * -4}px; + display: grid; + grid-template-columns: repeat(auto-fit, minmax(31%, max-content)); + ${[mq[3]]} { + grid-template-columns: repeat(auto-fit, minmax(31%, max-content)); + } + ${[mq[2]]} { + grid-template-columns: repeat(auto-fit, minmax(42%, max-content)); + } + ${[mq[1]]} { + grid-template-columns: repeat(auto-fit, minmax(63%, max-content)); + } + grid-gap: ${({ theme }) => theme.gridUnit * 8}px; + justify-content: left; + padding: ${({ theme }) => theme.gridUnit * 2}px + ${({ theme }) => theme.gridUnit * 4}px; + .ant-card-meta-avatar { + margin-top: ${({ theme }) => theme.gridUnit * 3}px; + margin-left: ${({ theme }) => theme.gridUnit * 2}px; + } + .ant-card-meta-title { + font-weight: ${({ theme }) => theme.typography.weights.bold}; + } +`; + +export default function ActivityTable({ user }: ActivityProps) { + const [activityData, setActivityData] = useState<ActivityData>({}); + const [loading, setLoading] = useState(true); + const [activeChild, setActiveChild] = useState('Viewed'); + // this api uses log for data which in some cases can be empty + const recent = `/superset/recent_activity/${user.userId}/?limit=5`; Review comment: This API seems broken. I just created a fix: https://github.com/apache/incubator-superset/pull/11481 You probably want to set this limit to 6 so that you can fill two full rows.  ########## 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 ActivityObjects { + action?: string; + item_title?: string; + slice_name: string; + time: string; + changed_on_utc: string; + url: string; + sql: string; + dashboard_title: string; + label: string; + id: string; + table: object; + item_url: string; +} + +interface ActivityProps { + user: { + userId: string | number; + }; +} + +interface ActivityData { + Created?: Array<object>; + Edited?: Array<object>; + Viewed?: Array<object>; + Examples?: Array<object>; +} + +const ActivityContainer = styled.div` + margin-left: ${({ theme }) => theme.gridUnit * 2}px; + margin-top: ${({ theme }) => theme.gridUnit * -4}px; + display: grid; + grid-template-columns: repeat(auto-fit, minmax(31%, max-content)); + ${[mq[3]]} { + grid-template-columns: repeat(auto-fit, minmax(31%, max-content)); + } + ${[mq[2]]} { + grid-template-columns: repeat(auto-fit, minmax(42%, max-content)); + } + ${[mq[1]]} { + grid-template-columns: repeat(auto-fit, minmax(63%, max-content)); + } + grid-gap: ${({ theme }) => theme.gridUnit * 8}px; + justify-content: left; + padding: ${({ theme }) => theme.gridUnit * 2}px + ${({ theme }) => theme.gridUnit * 4}px; + .ant-card-meta-avatar { + margin-top: ${({ theme }) => theme.gridUnit * 3}px; + margin-left: ${({ theme }) => theme.gridUnit * 2}px; + } + .ant-card-meta-title { + font-weight: ${({ theme }) => theme.typography.weights.bold}; + } +`; + +export default function ActivityTable({ user }: ActivityProps) { + const [activityData, setActivityData] = useState<ActivityData>({}); + const [loading, setLoading] = useState(true); + const [activeChild, setActiveChild] = useState('Viewed'); + // this api uses log for data which in some cases can be empty + const recent = `/superset/recent_activity/${user.userId}/?limit=5`; + + const getFilterTitle = (e: ActivityObjects) => { + if (e.dashboard_title) return e.dashboard_title; + if (e.label) return e.label; + if (e.url && !e.table) return e.item_title; + if (e.item_title) return e.item_title; + return e.slice_name; + }; + + const getIconName = (e: ActivityObjects) => { + if (e.sql) return 'sql'; + if (e.url?.includes('dashboard')) { + return 'nav-dashboard'; + } + if (e.url?.includes('explore') || e.item_url?.includes('explore')) { + return 'nav-charts'; + } + return ''; + }; + + const tabs = [ + { + name: 'Edited', + label: t('Edited'), + onClick: () => { + setActiveChild('Edited'); + }, + }, + { + name: 'Created', + label: t('Created'), + onClick: () => { + setActiveChild('Created'); + }, + }, + ]; + + if (activityData.Viewed) { + tabs.unshift({ + name: 'Viewed', + label: t('Viewed'), + onClick: () => { + setActiveChild('Viewed'); + }, + }); + } else { + tabs.unshift({ + name: 'Examples', + label: t('Examples'), + onClick: () => { + setActiveChild('Examples'); + }, + }); + } + + useEffect(() => { + getRecentAcitivtyObjs(user.userId, recent, addDangerToast) + .then(res => { + const data: any = { + Created: [ + ...res.createdByChart, + ...res.createdByDash, + ...res.createdByQuery, + ], + Edited: [...res.editedChart, ...res.editedDash], + }; + if (res.viewed) { + const filtered = reject(res.viewed, ['item_url', null]).map(r => r); + data.Viewed = filtered; + setActiveChild('Viewed'); + } else { + data.Examples = res.examples; + setActiveChild('Examples'); + } + setActivityData(data); Review comment:  After switching the tabs back and forth multiple times, I'm getting duplicate cards. You might want to test and debug your this hook a little bit more. I'm also not sure why some cards have icons, some do not. It would also be nice if the whole card can be clickable instead of just the title. ########## File path: superset-frontend/src/views/CRUD/welcome/EmptyState.tsx ########## @@ -0,0 +1,144 @@ +/** + * 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"> + {(() => { + if (tab === 'Viewed') { + return t( + `Recently viewed charts, dashboards, and saved queries will appear here`, + ); + } + if (tab === 'Created') { + return t( + 'Recently created charts, dashboards, and saved queries will appear here', + ); + } + if (tab === 'Examples') { + return t( + `Recent example charts, dashboards, and saved queries will appear here`, + ); + } + if (tab === 'Edited') { + return t( + `Recently edited charts, dashboards, and saved queries will appear here`, + ); + } + return null; + })()} + </div> + ); + // Mine and Recent Activity(all tabs) tab empty state + if (tab === 'Mine' || tableName === 'RECENTS') { Review comment: Nit: can we make the tab name all lowercase to have some consistency? ``` if (tab === 'mine' || tableName === 'recents') { ``` The idea is to not have to think about which case to use. ########## File path: superset-frontend/src/views/CRUD/welcome/DashboardTable.tsx ########## @@ -16,169 +16,176 @@ * specific language governing permissions and limitations * under the License. */ -import React from 'react'; -import { t, SupersetClient } from '@superset-ui/core'; -import { debounce } from 'lodash'; -import ListView, { FetchDataConfig } from 'src/components/ListView'; +import React, { useEffect, useState } from 'react'; +import { SupersetClient, t } from '@superset-ui/core'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { Dashboard, DashboardTableProps } from 'src/views/CRUD/types'; import withToasts from 'src/messageToasts/enhancers/withToasts'; -import { Dashboard } from 'src/types/bootstrapTypes'; +import PropertiesModal from 'src/dashboard/components/PropertiesModal'; +import DashboardCard from 'src/views/CRUD/dashboard/DashboardCard'; +import SubMenu from 'src/components/Menu/SubMenu'; +import Icon from 'src/components/Icon'; +import EmptyState from './EmptyState'; +import { createErrorHandler, CardContainer, IconContainer } from '../utils'; -const PAGE_SIZE = 25; +const PAGE_SIZE = 3; -interface DashboardTableProps { - addDangerToast: (message: string) => void; - search?: string; +export interface FilterValue { + col: string; + operator: string; + value: string | boolean | number | null | undefined; } -interface DashboardTableState { - dashboards: Dashboard[]; - dashboard_count: number; - loading: boolean; -} +function DashboardTable({ + user, + addDangerToast, + addSuccessToast, +}: DashboardTableProps) { + const { + state: { loading, resourceCollection: dashboards, bulkSelectEnabled }, + setResourceCollection: setDashboards, + hasPerm, + refreshData, + fetchData, + } = useListViewResource<Dashboard>( + 'dashboard', + t('dashboard'), + addDangerToast, + ); -class DashboardTable extends React.PureComponent< - DashboardTableProps, - DashboardTableState -> { - columns = [ - { - accessor: 'dashboard_title', - Header: 'Dashboard', - Cell: ({ - row: { - original: { url, dashboard_title: dashboardTitle }, - }, - }: { - row: { - original: { - url: string; - dashboard_title: string; - }; - }; - }) => <a href={url}>{dashboardTitle}</a>, - }, - { - accessor: 'changed_by.first_name', - Header: 'Modified By', - Cell: ({ - row: { - original: { changed_by_name: changedByName, changedByUrl }, - }, - }: { - row: { - original: { - changed_by_name: string; - changedByUrl: string; - }; - }; - }) => <a href={changedByUrl}>{changedByName}</a>, - }, - { - accessor: 'changed_on_delta_humanized', - Header: 'Modified', - Cell: ({ - row: { - original: { changed_on_delta_humanized: changedOn }, - }, - }: { - row: { - original: { - changed_on_delta_humanized: string; - }; - }; - }) => <span className="no-wrap">{changedOn}</span>, - }, - ]; + const [editModal, setEditModal] = useState<Dashboard | null>(null); + const [dashboardFilter, setDashboardFilter] = useState('Favorite'); - initialSort = [{ id: 'changed_on_delta_humanized', desc: true }]; + const handleDashboardEdit = (edits: Dashboard) => { + return SupersetClient.get({ + endpoint: `/api/v1/dashboard/${edits.id}`, + }).then( + ({ json = {} }) => { + setDashboards( + dashboards.map(dashboard => { + if (dashboard.id === json.id) { + return json.result; + } + return dashboard; + }), + ); + }, + createErrorHandler(errMsg => + addDangerToast( + t('An error occurred while fetching dashboards: %s', errMsg), + ), + ), + ); + }; - constructor(props: DashboardTableProps) { - super(props); - this.state = { - dashboards: [], - dashboard_count: 0, - loading: false, - }; - } + const getFilters = () => { + const filters = []; - componentDidUpdate(prevProps: DashboardTableProps) { - if (prevProps.search !== this.props.search) { - this.fetchDataDebounced({ - pageSize: PAGE_SIZE, - pageIndex: 0, - sortBy: this.initialSort, - filters: [], + if (dashboardFilter === 'Mine') { + filters.push({ + id: 'owners', + operator: 'rel_m_m', + value: `${user?.userId}`, + }); + } else { + filters.push({ + id: 'id', + operator: 'dashboard_is_fav', + value: true, }); } + return filters; + }; + const subMenus = []; + if (dashboards.length > 0 && dashboardFilter === 'favorite') { + subMenus.push({ + name: 'Favorite', + label: t('Favorite'), + onClick: () => setDashboardFilter('Favorite'), + }); } - fetchData = ({ pageIndex, pageSize, sortBy, filters }: FetchDataConfig) => { - this.setState({ loading: true }); - const filterExps = Object.keys(filters) - .map(fk => ({ - col: fk, - opr: filters[fk].filterId, - value: filters[fk].filterValue, - })) - .concat( - this.props.search - ? [ - { - col: 'dashboard_title', - opr: 'ct', - value: this.props.search, - }, - ] - : [], - ); - - const queryParams = JSON.stringify({ - order_column: sortBy[0].id, - order_direction: sortBy[0].desc ? 'desc' : 'asc', - page: pageIndex, - page_size: pageSize, - ...(filterExps.length ? { filters: filterExps } : {}), + useEffect(() => { + fetchData({ + pageIndex: 0, + pageSize: PAGE_SIZE, + sortBy: [ + { + id: 'changed_on_delta_humanized', + desc: true, + }, + ], + filters: getFilters(), }); + }, [dashboardFilter]); - return SupersetClient.get({ - endpoint: `/api/v1/dashboard/?q=${queryParams}`, - }) - .then(({ json }) => { - this.setState({ dashboards: json.result, dashboard_count: json.count }); - }) - .catch(response => { - if (response.status === 401) { - this.props.addDangerToast( - t( - "You don't have the necessary permissions to load dashboards. Please contact your administrator.", + return ( + <> + <SubMenu + activeChild={dashboardFilter} + tabs={[ + { + name: 'Favorite', + label: t('Favorite'), + onClick: () => setDashboardFilter('Favorite'), + }, + { + name: 'Mine', + label: t('Mine'), + onClick: () => setDashboardFilter('Mine'), + }, + ]} + buttons={[ + { + name: ( + <IconContainer> + <Icon name="plus-small" /> Dashboard{' '} + </IconContainer> ), - ); - } else { - this.props.addDangerToast( - t('An error occurred while fetching Dashboards'), - ); - } - }) - .finally(() => this.setState({ loading: false })); - }; - - // sort-comp disabled because of conflict with no-use-before-define rule - // eslint-disable-next-line react/sort-comp - fetchDataDebounced = debounce(this.fetchData, 200); - - render() { - return ( - <ListView - columns={this.columns} - data={this.state.dashboards} - count={this.state.dashboard_count} - pageSize={PAGE_SIZE} - fetchData={this.fetchData} - loading={this.state.loading} - initialSort={this.initialSort} + buttonStyle: 'tertiary', + onClick: () => { + window.location.href = '/dashboard/new'; + }, + }, + { + name: 'View All »', + buttonStyle: 'link', + onClick: () => { + window.location.href = '/dashboard/list/'; + }, + }, + ]} /> - ); - } + {editModal && ( + <PropertiesModal + dashboardId={editModal?.id} + show + onHide={() => setEditModal(null)} + onSubmit={handleDashboardEdit} + /> + )} + {dashboards.length > 0 ? ( + <CardContainer> + {dashboards.map(e => ( + <DashboardCard + {...{ + dashboard: e, + hasPerm, + bulkSelectEnabled, + refreshData, + addDangerToast, + addSuccessToast, + loading, + openDashboardEditModal: dashboard => setEditModal(dashboard), + }} + /> + ))} + </CardContainer> + ) : ( + <EmptyState tableName="DASHBOARDS" tab={dashboardFilter} /> Review comment: Currently the`EmptyState` will render even if the data is still loading. You might want to make sure all sections have a valid loading state. ########## File path: superset-frontend/src/views/CRUD/welcome/EmptyState.tsx ########## @@ -0,0 +1,144 @@ +/** + * 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; Review comment: Can this be string literals? ``` tab?: 'viewed' | 'created' | 'examples' ``` ########## File path: superset-frontend/src/views/CRUD/welcome/SavedQueries.tsx ########## @@ -0,0 +1,260 @@ +/** + * 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 { t, SupersetClient, styled } from '@superset-ui/core'; +import withToasts from 'src/messageToasts/enhancers/withToasts'; +import { Dropdown, Menu } from 'src/common/components'; +import { useListViewResource, copyQueryLink } from 'src/views/CRUD/hooks'; +import ListViewCard from 'src/components/ListViewCard'; +import DeleteModal from 'src/components/DeleteModal'; +import Icon from 'src/components/Icon'; +import SubMenu from 'src/components/Menu/SubMenu'; +import EmptyState from './EmptyState'; + +import { IconContainer, CardContainer, createErrorHandler } from '../utils'; + +const PAGE_SIZE = 3; + +interface Query { + id?: number; + sql_tables?: Array<any>; + database?: { + database_name: string; + }; + rows?: string; + description?: string; + end_time?: string; + label?: string; +} + +interface SavedQueriesProps { + user: { + userId: string | number; + }; + queryFilter: string; + addDangerToast: (arg0: string) => void; + addSuccessToast: (arg0: string) => void; +} + +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 SavedQueries = ({ + user, + addDangerToast, + addSuccessToast, +}: SavedQueriesProps) => { + const { + state: { loading, resourceCollection: queries }, + hasPerm, + fetchData, + refreshData, + } = useListViewResource<Query>('saved_query', t('query'), addDangerToast); + const [queryFilter, setQueryFilter] = useState('Favorite'); + const [queryDeleteModal, setQueryDeleteModal] = useState(false); + const [currentlyEdited, setCurrentlyEdited] = useState<Query>({}); + + const canEdit = hasPerm('can_edit'); + const canDelete = hasPerm('can_delete'); + + const handleQueryDelete = ({ id, label }: Query) => { + SupersetClient.delete({ + endpoint: `/api/v1/saved_query/${id}`, + }).then( + () => { + refreshData(); + setQueryDeleteModal(false); + addSuccessToast(t('Deleted: %s', label)); + }, + createErrorHandler(errMsg => + addDangerToast(t('There was an issue deleting %s: %s', label, errMsg)), + ), + ); + }; + + const getFilters = () => { + const filters = []; + if (queryFilter === 'Mine') { + filters.push({ + id: 'created_by', + operator: 'rel_o_m', + value: `${user?.userId}`, + }); + } else { + filters.push({ + id: 'id', + operator: 'saved_query_is_fav', + value: true, + }); + } + return filters; + }; + + useEffect(() => { + fetchData({ + pageIndex: 0, + pageSize: PAGE_SIZE, + sortBy: [ + { + id: 'changed_on_delta_humanized', + desc: true, + }, + ], + filters: getFilters(), + }); + }, [queryFilter]); + + const renderMenu = (query: Query) => ( + <Menu> + {canEdit && ( + <Menu.Item + onClick={() => { + window.location.href = `/superset/sqllab?savedQueryId=${query.id}`; + }} + > + {t('Edit')} + </Menu.Item> Review comment: ```tsx <Menu.Item> <a href={`/superset/sqllab?savedQueryId=${query.id}`} target="_blank">{t('Edit')}</a> </Menu.Item> ``` Why not just a regular anchor element (just like [AntD's official example](https://ant.design/components/dropdown/))? It's more semantically correct and better for a11y. Users can also "cmd + click" to open things in new tabs if they want to. ########## File path: superset-frontend/src/views/CRUD/welcome/DashboardTable.tsx ########## @@ -16,169 +16,176 @@ * specific language governing permissions and limitations * under the License. */ -import React from 'react'; -import { t, SupersetClient } from '@superset-ui/core'; -import { debounce } from 'lodash'; -import ListView, { FetchDataConfig } from 'src/components/ListView'; +import React, { useEffect, useState } from 'react'; +import { SupersetClient, t } from '@superset-ui/core'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { Dashboard, DashboardTableProps } from 'src/views/CRUD/types'; import withToasts from 'src/messageToasts/enhancers/withToasts'; -import { Dashboard } from 'src/types/bootstrapTypes'; +import PropertiesModal from 'src/dashboard/components/PropertiesModal'; +import DashboardCard from 'src/views/CRUD/dashboard/DashboardCard'; +import SubMenu from 'src/components/Menu/SubMenu'; +import Icon from 'src/components/Icon'; +import EmptyState from './EmptyState'; +import { createErrorHandler, CardContainer, IconContainer } from '../utils'; -const PAGE_SIZE = 25; +const PAGE_SIZE = 3; -interface DashboardTableProps { - addDangerToast: (message: string) => void; - search?: string; +export interface FilterValue { + col: string; + operator: string; + value: string | boolean | number | null | undefined; } -interface DashboardTableState { - dashboards: Dashboard[]; - dashboard_count: number; - loading: boolean; -} +function DashboardTable({ + user, + addDangerToast, + addSuccessToast, +}: DashboardTableProps) { + const { + state: { loading, resourceCollection: dashboards, bulkSelectEnabled }, + setResourceCollection: setDashboards, + hasPerm, + refreshData, + fetchData, + } = useListViewResource<Dashboard>( + 'dashboard', + t('dashboard'), + addDangerToast, + ); -class DashboardTable extends React.PureComponent< - DashboardTableProps, - DashboardTableState -> { - columns = [ - { - accessor: 'dashboard_title', - Header: 'Dashboard', - Cell: ({ - row: { - original: { url, dashboard_title: dashboardTitle }, - }, - }: { - row: { - original: { - url: string; - dashboard_title: string; - }; - }; - }) => <a href={url}>{dashboardTitle}</a>, - }, - { - accessor: 'changed_by.first_name', - Header: 'Modified By', - Cell: ({ - row: { - original: { changed_by_name: changedByName, changedByUrl }, - }, - }: { - row: { - original: { - changed_by_name: string; - changedByUrl: string; - }; - }; - }) => <a href={changedByUrl}>{changedByName}</a>, - }, - { - accessor: 'changed_on_delta_humanized', - Header: 'Modified', - Cell: ({ - row: { - original: { changed_on_delta_humanized: changedOn }, - }, - }: { - row: { - original: { - changed_on_delta_humanized: string; - }; - }; - }) => <span className="no-wrap">{changedOn}</span>, - }, - ]; + const [editModal, setEditModal] = useState<Dashboard | null>(null); Review comment: I'm not sure about opening the Dashboard Properties modal with this link. <img src="https://user-images.githubusercontent.com/335541/97531800-34ef5b80-1972-11eb-874f-d735aef3d169.png"; width="300"> It should probably just open the dashboard page in edit mode in a new window instead (`/superset/dashboard/${dashboard.id}/?edit=true`), since editing dashboard layout/charts is a much more common action than editing dashboard properties. I would probably also change the order of the links to "Edit > Export > Delete". ---------------------------------------------------------------- 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]
![https://user-images.githubusercontent.com/335541/97531220-f907c680-1970-11eb-9929-8f83130092e8.png"](https://user-images.githubusercontent.com/335541/97531220-f907c680-1970-11eb-9929-8f83130092e8.png"){kind=link}
![https://user-images.githubusercontent.com/335541/97531553-a4b11680-1971-11eb-9f6f-5ca502973be2.png"](https://user-images.githubusercontent.com/335541/97531553-a4b11680-1971-11eb-9f6f-5ca502973be2.png"){kind=link}
![https://user-images.githubusercontent.com/335541/97532128-eb534080-1972-11eb-807e-a6e2e87f1213.png"](https://user-images.githubusercontent.com/335541/97532128-eb534080-1972-11eb-807e-a6e2e87f1213.png"){kind=link}
![https://user-images.githubusercontent.com/335541/97531800-34ef5b80-1972-11eb-874f-d735aef3d169.png"](https://user-images.githubusercontent.com/335541/97531800-34ef5b80-1972-11eb-874f-d735aef3d169.png"){kind=link}