feat(dashboard): Transition to Explore with React Router (#20606)

* feat(dashboard): Use react-router for transition to Explore + cmd click to open in new tab

* Update tooltip

* Add a feature flag

* Update edit chart onclick event

* Fix lint

* Fix tests

* Change feature flag name

* Add tooltip to Edit chart

UPDATING.md

@@ -28,6 +28,7 @@ assists people when migrating to a new version.
 - [18936](https://github.com/apache/superset/pull/18936): Removes legacy SIP-15 interim logic/flags—specifically the `SIP_15_ENABLED`, `SIP_15_GRACE_PERIOD_END`, `SIP_15_DEFAULT_TIME_RANGE_ENDPOINTS`, and `SIP_15_TOAST_MESSAGE` flags. Time range endpoints are no longer configurable and strictly adhere to the `[start, end)` paradigm, i.e., inclusive of the start and exclusive of the end. Additionally this change removes the now obsolete `time_range_endpoints` from the form-data and resulting in the cache being busted.
 - [19570](https://github.com/apache/superset/pull/19570): makes [sqloxide](https://pypi.org/project/sqloxide/) optional so the SIP-68 migration can be run on aarch64. If the migration is taking too long installing sqloxide manually should improve the performance.
 - [20170](https://github.com/apache/superset/pull/20170): Introduced a new endpoint for getting datasets samples.
+- [20606](https://github.com/apache/superset/pull/20606): When user clicks on chart title or "Edit chart" button in Dashboard page, Explore opens in the same tab. Clicking while holding cmd/ctrl opens Explore in a new tab. To bring back the old behaviour (always opening Explore in a new tab), flip feature flag `DASHBOARD_EDIT_CHART_IN_NEW_TAB` to `True`.
 
 ### Breaking Changes
 

superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts

@@ -55,6 +55,7 @@ export enum FeatureFlag {
   UX_BETA = 'UX_BETA',
   GENERIC_CHART_AXES = 'GENERIC_CHART_AXES',
   USE_ANALAGOUS_COLORS = 'USE_ANALAGOUS_COLORS',
+  DASHBOARD_EDIT_CHART_IN_NEW_TAB = 'DASHBOARD_EDIT_CHART_IN_NEW_TAB',
 }
 export type ScheduleQueriesProps = {
   JSONSCHEMA: {

superset-frontend/src/components/EditableTitle/index.tsx

@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React, { useEffect, useState, useRef } from 'react';
+import React, { MouseEvent, useEffect, useState, useRef } from 'react';
 import cx from 'classnames';
 import { css, styled, t } from '@superset-ui/core';
 import { Tooltip } from 'src/components/Tooltip';
@@ -37,7 +37,7 @@ export interface EditableTitleProps {
   placeholder?: string;
   certifiedBy?: string;
   certificationDetails?: string;
-  onClickTitle?: () => void;
+  onClickTitle?: (event: MouseEvent) => void;
 }
 
 const StyledCertifiedBadge = styled(CertifiedBadge)`

superset-frontend/src/dashboard/components/SliceHeader/SliceHeader.test.tsx

@@ -164,7 +164,7 @@ const createProps = (overrides: any = {}) => ({
 
 test('Should render', () => {
   const props = createProps();
-  render(<SliceHeader {...props} />, { useRedux: true });
+  render(<SliceHeader {...props} />, { useRedux: true, useRouter: true });
   expect(screen.getByTestId('slice-header')).toBeInTheDocument();
 });
 
@@ -270,15 +270,41 @@ test('Should render click to edit prompt and run onExploreChart on click', async
   render(<SliceHeader {...props} />, { useRedux: true });
   userEvent.hover(screen.getByText('Vaccine Candidates per Phase'));
   expect(
-    await screen.findByText(
-      'Click to edit Vaccine Candidates per Phase in a new tab',
-    ),
+    await screen.findByText('Click to edit Vaccine Candidates per Phase.'),
+  ).toBeInTheDocument();
+  expect(
+    await screen.findByText('Use ctrl + click to open in a new tab.'),
   ).toBeInTheDocument();
 
   userEvent.click(screen.getByText('Vaccine Candidates per Phase'));
   expect(props.onExploreChart).toHaveBeenCalled();
 });
 
+test('Display cmd button in tooltip if running on MacOS', async () => {
+  jest.spyOn(window.navigator, 'appVersion', 'get').mockReturnValue('Mac');
+  const props = createProps();
+  render(<SliceHeader {...props} />, { useRedux: true });
+  userEvent.hover(screen.getByText('Vaccine Candidates per Phase'));
+  expect(
+    await screen.findByText('Click to edit Vaccine Candidates per Phase.'),
+  ).toBeInTheDocument();
+  expect(
+    await screen.findByText('Use ⌘ + click to open in a new tab.'),
+  ).toBeInTheDocument();
+});
+
+test('Display correct tooltip when DASHBOARD_EDIT_CHART_IN_NEW_TAB is enabled', async () => {
+  window.featureFlags.DASHBOARD_EDIT_CHART_IN_NEW_TAB = true;
+  const props = createProps();
+  render(<SliceHeader {...props} />, { useRedux: true });
+  userEvent.hover(screen.getByText('Vaccine Candidates per Phase'));
+  expect(
+    await screen.findByText(
+      'Click to edit Vaccine Candidates per Phase in a new tab',
+    ),
+  ).toBeInTheDocument();
+});
+
 test('Should not render click to edit prompt and run onExploreChart on click if supersetCanExplore=false', () => {
   const props = createProps({ supersetCanExplore: false });
   render(<SliceHeader {...props} />, { useRedux: true });

superset-frontend/src/dashboard/components/SliceHeader/index.tsx

@@ -16,7 +16,14 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React, { FC, useEffect, useMemo, useRef, useState } from 'react';
+import React, {
+  FC,
+  ReactNode,
+  useEffect,
+  useMemo,
+  useRef,
+  useState,
+} from 'react';
 import { styled, t } from '@superset-ui/core';
 import { useUiConfig } from 'src/components/UiConfigContext';
 import { Tooltip } from 'src/components/Tooltip';
@@ -29,6 +36,7 @@ import FiltersBadge from 'src/dashboard/components/FiltersBadge';
 import Icons from 'src/components/Icons';
 import { RootState } from 'src/dashboard/types';
 import FilterIndicator from 'src/dashboard/components/FiltersBadge/FilterIndicator';
+import { getSliceHeaderTooltip } from 'src/dashboard/util/getSliceHeaderTooltip';
 import { clearDataMask } from 'src/dataMask/actions';
 
 type SliceHeaderProps = SliceHeaderControlsProps & {
@@ -89,7 +97,7 @@ const SliceHeader: FC<SliceHeaderProps> = ({
 }) => {
   const dispatch = useDispatch();
   const uiConfig = useUiConfig();
-  const [headerTooltip, setHeaderTooltip] = useState<string | null>(null);
+  const [headerTooltip, setHeaderTooltip] = useState<ReactNode | null>(null);
   const headerRef = useRef<HTMLDivElement>(null);
   // TODO: change to indicator field after it will be implemented
   const crossFilterValue = useSelector<RootState, any>(
@@ -110,11 +118,7 @@ const SliceHeader: FC<SliceHeaderProps> = ({
   useEffect(() => {
     const headerElement = headerRef.current;
     if (handleClickTitle) {
-      setHeaderTooltip(
-        sliceName
-          ? t('Click to edit %s in a new tab', sliceName)
-          : t('Click to edit chart in a new tab'),
-      );
+      setHeaderTooltip(getSliceHeaderTooltip(sliceName));
     } else if (
       headerElement &&
       (headerElement.scrollWidth > headerElement.offsetWidth ||

superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx

@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import React from 'react';
+import React, { MouseEvent, Key } from 'react';
 import moment from 'moment';
 import {
   Behavior,
@@ -32,6 +32,8 @@ import ShareMenuItems from 'src/dashboard/components/menu/ShareMenuItems';
 import downloadAsImage from 'src/utils/downloadAsImage';
 import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags';
 import CrossFilterScopingModal from 'src/dashboard/components/CrossFilterScopingModal/CrossFilterScopingModal';
+import { getSliceHeaderTooltip } from 'src/dashboard/util/getSliceHeaderTooltip';
+import { Tooltip } from 'src/components/Tooltip';
 import Icons from 'src/components/Icons';
 import ModalTrigger from 'src/components/ModalTrigger';
 import Button from 'src/components/Button';
@@ -106,7 +108,7 @@ export interface SliceHeaderControlsProps {
   isFullSize?: boolean;
   isDescriptionExpanded?: boolean;
   formData: QueryFormData;
-  onExploreChart: () => void;
+  onExploreChart: (event: MouseEvent) => void;
 
   forceRefresh: (sliceId: number, dashboardId: number) => void;
   logExploreChart?: (sliceId: number) => void;
@@ -170,8 +172,8 @@ class SliceHeaderControls extends React.PureComponent<
     key,
     domEvent,
   }: {
-    key: React.Key;
-    domEvent: React.MouseEvent<HTMLElement>;
+    key: Key;
+    domEvent: MouseEvent<HTMLElement>;
   }) {
     switch (key) {
       case MENU_KEYS.FORCE_REFRESH:
@@ -306,9 +308,11 @@ class SliceHeaderControls extends React.PureComponent<
         {this.props.supersetCanExplore && (
           <Menu.Item
             key={MENU_KEYS.EXPLORE_CHART}
-            onClick={this.props.onExploreChart}
+            onClick={({ domEvent }) => this.props.onExploreChart(domEvent)}
           >
-            {t('Edit chart')}
+            <Tooltip title={getSliceHeaderTooltip(this.props.slice.slice_name)}>
+              {t('Edit chart')}
+            </Tooltip>
           </Menu.Item>
         )}
 

superset-frontend/src/dashboard/components/gridComponents/Chart.jsx

@@ -19,8 +19,15 @@
 import cx from 'classnames';
 import React from 'react';
 import PropTypes from 'prop-types';
-import { styled, t, logging } from '@superset-ui/core';
+import {
+  styled,
+  t,
+  logging,
+  isFeatureEnabled,
+  FeatureFlag,
+} from '@superset-ui/core';
 import { isEqual } from 'lodash';
+import { withRouter } from 'react-router-dom';
 
 import { exportChart, mountExploreUrl } from 'src/explore/exploreUtils';
 import ChartContainer from 'src/components/Chart/ChartContainer';
@@ -120,7 +127,7 @@ const SliceContainer = styled.div`
   max-height: 100%;
 `;
 
-export default class Chart extends React.Component {
+class Chart extends React.Component {
   constructor(props) {
     super(props);
     this.state = {
@@ -270,7 +277,9 @@ export default class Chart extends React.Component {
     });
   };
 
-  onExploreChart = async () => {
+  onExploreChart = async clickEvent => {
+    const isOpenInNewTab =
+      clickEvent.shiftKey || clickEvent.ctrlKey || clickEvent.metaKey;
     try {
       const lastTabId = window.localStorage.getItem('last_tab_id');
       const nextTabId = lastTabId
@@ -287,7 +296,14 @@ export default class Chart extends React.Component {
         [URL_PARAMS.formDataKey.name]: key,
         [URL_PARAMS.sliceId.name]: this.props.slice.slice_id,
       });
-      window.open(url, '_blank', 'noreferrer');
+      if (
+        isFeatureEnabled(FeatureFlag.DASHBOARD_EDIT_CHART_IN_NEW_TAB) ||
+        isOpenInNewTab
+      ) {
+        window.open(url, '_blank', 'noreferrer');
+      } else {
+        this.props.history.push(url);
+      }
     } catch (error) {
       logging.error(error);
       this.props.addDangerToast(t('An error occurred while opening Explore'));
@@ -496,3 +512,5 @@ export default class Chart extends React.Component {
 
 Chart.propTypes = propTypes;
 Chart.defaultProps = defaultProps;
+
+export default withRouter(Chart);

superset-frontend/src/dashboard/components/gridComponents/Chart.test.jsx

@@ -72,7 +72,9 @@ describe('Chart', () => {
   };
 
   function setup(overrideProps) {
-    const wrapper = shallow(<Chart {...props} {...overrideProps} />);
+    const wrapper = shallow(
+      <Chart.WrappedComponent {...props} {...overrideProps} />,
+    );
     return wrapper;
   }
 
@@ -94,7 +96,10 @@ describe('Chart', () => {
   });
 
   it('should calculate the description height if it has one and isExpanded=true', () => {
-    const spy = jest.spyOn(Chart.prototype, 'getDescriptionHeight');
+    const spy = jest.spyOn(
+      Chart.WrappedComponent.prototype,
+      'getDescriptionHeight',
+    );
     const wrapper = setup({ isExpanded: true });
 
     expect(wrapper.find('.slice_description')).toExist();

superset-frontend/src/dashboard/components/gridComponents/ChartHolder.test.tsx

@@ -18,15 +18,12 @@
  */
 
 import React from 'react';
+import mockState from 'spec/fixtures/mockState';
 import { sliceId as chartId } from 'spec/fixtures/mockChartQueries';
+import { screen, render } from 'spec/helpers/testing-library';
 import { nativeFiltersInfo } from 'src/dashboard/fixtures/mockNativeFilters';
 import newComponentFactory from 'src/dashboard/util/newComponentFactory';
-import { getMockStore } from 'spec/fixtures/mockStore';
 import { initialState } from 'src/SqlLab/fixtures';
-import { DndProvider } from 'react-dnd';
-import { HTML5Backend } from 'react-dnd-html5-backend';
-import { Provider } from 'react-redux';
-import { screen, render } from 'spec/helpers/testing-library';
 import { CHART_TYPE, ROW_TYPE } from '../../util/componentTypes';
 import { ChartHolder } from './index';
 
@@ -67,17 +64,14 @@ describe('ChartHolder', () => {
     fullSizeChartId: chartId,
     setFullSizeChartId: () => {},
   };
-  const mockStore = getMockStore({
-    ...initialState,
-  });
+
   const renderWrapper = () =>
-    render(
-      <Provider store={mockStore}>
-        <DndProvider backend={HTML5Backend}>
-          <ChartHolder {...defaultProps} />{' '}
-        </DndProvider>
-      </Provider>,
-    );
+    render(<ChartHolder {...defaultProps} />, {
+      useRouter: true,
+      useDnd: true,
+      useRedux: true,
+      initialState: { ...mockState, ...initialState },
+    });
 
   it('should render empty state', async () => {
     renderWrapper();

superset-frontend/src/dashboard/components/gridComponents/Column.test.jsx

@@ -20,6 +20,7 @@ import { Provider } from 'react-redux';
 import React from 'react';
 import { mount } from 'enzyme';
 import sinon from 'sinon';
+import { MemoryRouter } from 'react-router-dom';
 import { supersetTheme, ThemeProvider } from '@superset-ui/core';
 import { DndProvider } from 'react-dnd';
 import { HTML5Backend } from 'react-dnd-html5-backend';
@@ -71,9 +72,11 @@ describe('Column', () => {
     });
     const wrapper = mount(
       <Provider store={mockStore}>
-        <DndProvider backend={HTML5Backend}>
-          <Column {...props} {...overrideProps} />
-        </DndProvider>
+        <MemoryRouter>
+          <DndProvider backend={HTML5Backend}>
+            <Column {...props} {...overrideProps} />
+          </DndProvider>
+        </MemoryRouter>
       </Provider>,
       {
         wrappingComponent: ThemeProvider,

superset-frontend/src/dashboard/components/gridComponents/Row.test.jsx

@@ -22,6 +22,7 @@ import { mount } from 'enzyme';
 import sinon from 'sinon';
 import { DndProvider } from 'react-dnd';
 import { HTML5Backend } from 'react-dnd-html5-backend';
+import { MemoryRouter } from 'react-router-dom';
 
 import BackgroundStyleDropdown from 'src/dashboard/components/menu/BackgroundStyleDropdown';
 import DashboardComponent from 'src/dashboard/containers/DashboardComponent';
@@ -67,9 +68,11 @@ describe('Row', () => {
     });
     const wrapper = mount(
       <Provider store={mockStore}>
-        <DndProvider backend={HTML5Backend}>
-          <Row {...props} {...overrideProps} />
-        </DndProvider>
+        <MemoryRouter>
+          <DndProvider backend={HTML5Backend}>
+            <Row {...props} {...overrideProps} />
+          </DndProvider>
+        </MemoryRouter>
       </Provider>,
       {
         wrappingComponent: ThemeProvider,

superset-frontend/src/dashboard/util/getSliceHeaderTooltip.tsx

@@ -0,0 +1,44 @@
+/**
+ * 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 { FeatureFlag, isFeatureEnabled, t } from '@superset-ui/core';
+import { detectOS } from 'src/utils/common';
+
+export const getSliceHeaderTooltip = (sliceName: string | undefined) => {
+  if (isFeatureEnabled(FeatureFlag.DASHBOARD_EDIT_CHART_IN_NEW_TAB)) {
+    return sliceName
+      ? t('Click to edit %s in a new tab', sliceName)
+      : t('Click to edit chart.');
+  }
+  const isMac = detectOS() === 'MacOS';
+  const firstLine = sliceName
+    ? t('Click to edit %s.', sliceName)
+    : t('Click to edit chart.');
+  const secondLine = t(
+    'Use %s to open in a new tab.',
+    isMac ? '⌘ + click' : 'ctrl + click',
+  );
+  return (
+    <>
+      <div>{firstLine}</div>
+      <div>{secondLine}</div>
+    </>
+  );
+};

superset/config.py

@@ -429,6 +429,7 @@ DEFAULT_FEATURE_FLAGS: Dict[str, bool] = {
     "GENERIC_CHART_AXES": False,
     "ALLOW_ADHOC_SUBQUERY": False,
     "USE_ANALAGOUS_COLORS": True,
+    "DASHBOARD_EDIT_CHART_IN_NEW_TAB": False,
     # Apply RLS rules to SQL Lab queries. This requires parsing and manipulating the
     # query, and might break queries and/or allow users to bypass RLS. Use with care!
     "RLS_IN_SQLLAB": False,