Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: save active tabs in dashboard permalink #19983

Merged
merged 2 commits into from
Jun 29, 2022
Merged

feat: save active tabs in dashboard permalink #19983

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dbe28d1
feat: save active tabs in dashboard permalink
ktmud May 7, 2022
a3076c1
fix unit test
ktmud Jun 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 21 additions & 23 deletions superset-frontend/src/dashboard/actions/hydrate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,23 +59,23 @@ import { updateColorSchema } from './dashboardInfo';
export const HYDRATE_DASHBOARD = 'HYDRATE_DASHBOARD';

export const hydrateDashboard =
(
dashboardData,
chartData,
({
dashboard,
charts,
filterboxMigrationState = FILTER_BOX_MIGRATION_STATES.NOOP,
dataMaskApplied,
) =>
dataMask,
activeTabs,
}) =>
(dispatch, getState) => {
const { user, common, dashboardState } = getState();

const { metadata } = dashboardData;
const { metadata, position_data: positionData } = dashboard;
const regularUrlParams = extractUrlParams('regular');
const reservedUrlParams = extractUrlParams('reserved');
const editMode = reservedUrlParams.edit === 'true';

let preselectFilters = {};

chartData.forEach(chart => {
charts.forEach(chart => {
// eslint-disable-next-line no-param-reassign
chart.slice_id = chart.form_data.slice_id;
});
Expand All @@ -98,12 +98,10 @@ export const hydrateDashboard =
updateColorSchema(metadata, metadata?.label_colors);
}

// dashboard layout
const { position_data } = dashboardData;
// new dash: position_json could be {} or null
const layout =
position_data && Object.keys(position_data).length > 0
? position_data
positionData && Object.keys(positionData).length > 0
? positionData
: getEmptyLayout();

// create a lookup to sync layout names with slice names
Expand All @@ -128,7 +126,7 @@ export const hydrateDashboard =
const sliceIds = new Set();
const slicesFromExploreCount = new Map();

chartData.forEach(slice => {
charts.forEach(slice => {
const key = slice.slice_id;
const form_data = {
...slice.form_data,
Expand Down Expand Up @@ -269,7 +267,7 @@ export const hydrateDashboard =
id: DASHBOARD_HEADER_ID,
type: DASHBOARD_HEADER_TYPE,
meta: {
text: dashboardData.dashboard_title,
text: dashboard.dashboard_title,
},
};

Expand All @@ -291,7 +289,7 @@ export const hydrateDashboard =
let filterConfig = metadata?.native_filter_configuration || [];
if (filterboxMigrationState === FILTER_BOX_MIGRATION_STATES.REVIEWING) {
filterConfig = getNativeFilterConfig(
chartData,
charts,
filterScopes,
preselectFilters,
);
Expand All @@ -302,7 +300,7 @@ export const hydrateDashboard =
filterConfig,
});
metadata.show_native_filters =
dashboardData?.metadata?.show_native_filters ??
dashboard?.metadata?.show_native_filters ??
(isFeatureEnabled(FeatureFlag.DASHBOARD_NATIVE_FILTERS) &&
[
FILTER_BOX_MIGRATION_STATES.CONVERTED,
Expand Down Expand Up @@ -343,7 +341,7 @@ export const hydrateDashboard =
}

const { roles } = user;
const canEdit = canUserEditDashboard(dashboardData, user);
const canEdit = canUserEditDashboard(dashboard, user);

return dispatch({
type: HYDRATE_DASHBOARD,
Expand All @@ -352,7 +350,7 @@ export const hydrateDashboard =
charts: chartQueries,
// read-only data
dashboardInfo: {
...dashboardData,
...dashboard,
metadata,
userId: user.userId ? String(user.userId) : null, // legacy, please use state.user instead
dash_edit_perm: canEdit,
Expand Down Expand Up @@ -380,7 +378,7 @@ export const hydrateDashboard =
conf: common?.conf,
},
},
dataMask: dataMaskApplied,
dataMask,
dashboardFilters,
nativeFilters,
dashboardState: {
Expand All @@ -394,17 +392,17 @@ export const hydrateDashboard =
// dashboard viewers can set refresh frequency for the current visit,
// only persistent refreshFrequency will be saved to backend
shouldPersistRefreshFrequency: false,
css: dashboardData.css || '',
css: dashboard.css || '',
colorNamespace: metadata?.color_namespace || null,
colorScheme: metadata?.color_scheme || null,
editMode: canEdit && editMode,
isPublished: dashboardData.published,
isPublished: dashboard.published,
hasUnsavedChanges: false,
maxUndoHistoryExceeded: false,
lastModifiedTime: dashboardData.changed_on,
lastModifiedTime: dashboard.changed_on,
isRefreshing: false,
isFiltersRefreshing: false,
activeTabs: dashboardState?.activeTabs || [],
activeTabs: activeTabs || dashboardState?.activeTabs || [],
filterboxMigrationState,
datasetsStatus: ResourceStatus.LOADING,
},
Expand Down
34 changes: 25 additions & 9 deletions superset-frontend/src/dashboard/components/URLShortLinkButton/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,11 @@ import React, { useState } from 'react';
import { t } from '@superset-ui/core';
import Popover, { PopoverProps } from 'src/components/Popover';
import CopyToClipboard from 'src/components/CopyToClipboard';
import { getDashboardPermalink, getUrlParam } from 'src/utils/urlUtils';
import { getDashboardPermalink } from 'src/utils/urlUtils';
import { useToasts } from 'src/components/MessageToasts/withToasts';
import { URL_PARAMS } from 'src/constants';
import { getFilterValue } from 'src/dashboard/components/nativeFilters/FilterBar/keyValue';
import { useSelector } from 'react-redux';
import { RootState } from 'src/dashboard/types';
import { getClientErrorObject } from 'src/utils/getClientErrorObject';

export type URLShortLinkButtonProps = {
dashboardId: number;
Expand All @@ -42,19 +43,27 @@ export default function URLShortLinkButton({
}: URLShortLinkButtonProps) {
const [shortUrl, setShortUrl] = useState('');
const { addDangerToast } = useToasts();
const { dataMask, activeTabs } = useSelector((state: RootState) => ({
dataMask: state.dataMask,
activeTabs: state.dashboardState.activeTabs,
}));

const getCopyUrl = async () => {
const nativeFiltersKey = getUrlParam(URL_PARAMS.nativeFiltersKey);
try {
const filterState = await getFilterValue(dashboardId, nativeFiltersKey);
const url = await getDashboardPermalink({
dashboardId,
filterState,
hash: anchorLinkId,
dataMask,
activeTabs,
anchor: anchorLinkId,
});
setShortUrl(url);
} catch (error) {
addDangerToast(error);
if (error) {
addDangerToast(
(await getClientErrorObject(error)).error ||
t('Something went wrong.'),
);
}
}
};

Expand All @@ -66,7 +75,14 @@ export default function URLShortLinkButton({
trigger="click"
placement={placement}
content={
<div id="shorturl-popover" data-test="shorturl-popover">
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, shouldn't the stop propagation go on the copy to clipboard link instead? It's weird that we need to handle a click on a div

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The click event also triggers popover open. Since this is related to the popover, I thought it'd make more sense to handle the event on the direct children of the popover trigger. If we stop propagation on the icon, the popover may not even open.

<div
id="shorturl-popover"
data-test="shorturl-popover"
onClick={e => {
e.stopPropagation();
}}
>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stop clicking on the copy icon from switching tabs, which is kind of surprising/annoying----if clicking on the link icon doesn't switch tabs, then clicking on things in the popover triggered by the link icon shouldn't switch tabs either.

Before

tab-copy-click-before

After

tab-copy-click-after

<CopyToClipboard
text={shortUrl}
copyNode={
Expand Down
12 changes: 11 additions & 1 deletion superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const propTypes = {
editMode: PropTypes.bool.isRequired,
renderHoverMenu: PropTypes.bool,
directPathToChild: PropTypes.arrayOf(PropTypes.string),
activeTabs: PropTypes.arrayOf(PropTypes.string),
filterboxMigrationState: FILTER_BOX_MIGRATION_STATES,

// actions (from DashboardComponent.jsx)
Expand All @@ -74,6 +75,7 @@ const defaultProps = {
renderHoverMenu: true,
availableColumnCount: 0,
columnWidth: 0,
activeTabs: [],
directPathToChild: [],
filterboxMigrationState: FILTER_BOX_MIGRATION_STATES.NOOP,
setActiveTabs() {},
Expand Down Expand Up @@ -112,13 +114,20 @@ const StyledTabsContainer = styled.div`
export class Tabs extends React.PureComponent {
constructor(props) {
super(props);
const tabIndex = Math.max(
let tabIndex = Math.max(
0,
findTabIndexByComponentId({
currentComponent: props.component,
directPathToChild: props.directPathToChild,
}),
);
if (tabIndex === 0 && props.activeTabs?.length) {
props.component.children.forEach((tabId, index) => {
if (tabIndex === 0 && props.activeTabs.includes(tabId)) {
tabIndex = index;
}
});
}
const { children: tabIds } = props.component;
const activeKey = tabIds[tabIndex];

Expand Down Expand Up @@ -408,6 +417,7 @@ Tabs.defaultProps = defaultProps;
function mapStateToProps(state) {
return {
nativeFilters: state.nativeFilters,
activeTabs: state.dashboardState.activeTabs,
directPathToChild: state.dashboardState.directPathToChild,
filterboxMigrationState: state.dashboardState.filterboxMigrationState,
};
Expand Down
5 changes: 5 additions & 0 deletions superset-frontend/src/dashboard/components/menu/ShareMenuItems/ShareMenuItems.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ test('Should render menu items', () => {
<Menu onClick={jest.fn()} selectable={false} data-test="main-menu">
<ShareMenuItems {...props} />
</Menu>,
{ useRedux: true },
);
expect(
screen.getByRole('menuitem', { name: 'Copy dashboard URL' }),
Expand All @@ -92,6 +93,7 @@ test('Click on "Copy dashboard URL" and succeed', async () => {
<Menu onClick={jest.fn()} selectable={false} data-test="main-menu">
<ShareMenuItems {...props} />
</Menu>,
{ useRedux: true },
);

await waitFor(() => {
Expand Down Expand Up @@ -119,6 +121,7 @@ test('Click on "Copy dashboard URL" and fail', async () => {
<Menu onClick={jest.fn()} selectable={false} data-test="main-menu">
<ShareMenuItems {...props} />
</Menu>,
{ useRedux: true },
);

await waitFor(() => {
Expand Down Expand Up @@ -147,6 +150,7 @@ test('Click on "Share dashboard by email" and succeed', async () => {
<Menu onClick={jest.fn()} selectable={false} data-test="main-menu">
<ShareMenuItems {...props} />
</Menu>,
{ useRedux: true },
);

await waitFor(() => {
Expand Down Expand Up @@ -177,6 +181,7 @@ test('Click on "Share dashboard by email" and fail', async () => {
<Menu onClick={jest.fn()} selectable={false} data-test="main-menu">
<ShareMenuItems {...props} />
</Menu>,
{ useRedux: true },
);

await waitFor(() => {
Expand Down
20 changes: 10 additions & 10 deletions superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ import React from 'react';
import copyTextToClipboard from 'src/utils/copy';
import { t, logging } from '@superset-ui/core';
import { Menu } from 'src/components/Menu';
import { getDashboardPermalink, getUrlParam } from 'src/utils/urlUtils';
import { URL_PARAMS } from 'src/constants';
import { getFilterValue } from 'src/dashboard/components/nativeFilters/FilterBar/keyValue';
import { getDashboardPermalink } from 'src/utils/urlUtils';
import { RootState } from 'src/dashboard/types';
import { useSelector } from 'react-redux';

interface ShareMenuItemProps {
url?: string;
Expand All @@ -48,17 +48,17 @@ const ShareMenuItems = (props: ShareMenuItemProps) => {
dashboardComponentId,
...rest
} = props;
const { dataMask, activeTabs } = useSelector((state: RootState) => ({
dataMask: state.dataMask,
activeTabs: state.dashboardState.activeTabs,
}));

async function generateUrl() {
const nativeFiltersKey = getUrlParam(URL_PARAMS.nativeFiltersKey);
let filterState = {};
if (nativeFiltersKey && dashboardId) {
filterState = await getFilterValue(dashboardId, nativeFiltersKey);
}
return getDashboardPermalink({
dashboardId,
filterState,
hash: dashboardComponentId,
dataMask,
activeTabs,
anchor: dashboardComponentId,
});
}

Expand Down
26 changes: 14 additions & 12 deletions superset-frontend/src/dashboard/containers/DashboardPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,13 @@ import {
import { URL_PARAMS } from 'src/constants';
import { getUrlParam } from 'src/utils/urlUtils';
import { canUserEditDashboard } from 'src/dashboard/util/permissionUtils';
import { getFilterSets } from '../actions/nativeFilters';
import { setDatasetsStatus } from '../actions/dashboardState';
import { getFilterSets } from 'src/dashboard/actions/nativeFilters';
import { setDatasetsStatus } from 'src/dashboard/actions/dashboardState';
import {
getFilterValue,
getPermalinkValue,
} from '../components/nativeFilters/FilterBar/keyValue';
import { filterCardPopoverStyle } from '../styles';
} from 'src/dashboard/components/nativeFilters/FilterBar/keyValue';
import { filterCardPopoverStyle } from 'src/dashboard/styles';

export const MigrationContext = React.createContext(
FILTER_BOX_MIGRATION_STATES.NOOP,
Expand Down Expand Up @@ -183,19 +183,20 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
async function getDataMaskApplied() {
const permalinkKey = getUrlParam(URL_PARAMS.permalinkKey);
const nativeFilterKeyValue = getUrlParam(URL_PARAMS.nativeFiltersKey);
let dataMaskFromUrl = nativeFilterKeyValue || {};

const isOldRison = getUrlParam(URL_PARAMS.nativeFilters);

let dataMask = nativeFilterKeyValue || {};
let activeTabs: string[] | undefined = [];
if (permalinkKey) {
const permalinkValue = await getPermalinkValue(permalinkKey);
if (permalinkValue) {
dataMaskFromUrl = permalinkValue.state.filterState;
({ dataMask, activeTabs } = permalinkValue.state);
}
} else if (nativeFilterKeyValue) {
dataMaskFromUrl = await getFilterValue(id, nativeFilterKeyValue);
dataMask = await getFilterValue(id, nativeFilterKeyValue);
}
if (isOldRison) {
dataMaskFromUrl = isOldRison;
dataMask = isOldRison;
}

if (readyToRender) {
Expand All @@ -207,12 +208,13 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
}
}
dispatch(
hydrateDashboard(
hydrateDashboard({
dashboard,
charts,
activeTabs,
filterboxMigrationState,
dataMaskFromUrl,
),
dataMask,
}),
);
}
return null;
Expand Down
Loading