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.

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(dashboard/native-filters): Hide filters out of scope of current tab #14933

Merged
merged 6 commits into from
Jun 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ describe('dashboard filters card view', () => {
cy.get('.Select__menu').contains('Published').click({ timeout: 5000 });
cy.get('[data-test="styled-card"]').should('have.length', 2);
cy.get('[data-test="styled-card"]')
.first()
.contains('USA Births Names')
.should('be.visible');
cy.get('.Select__control').eq(1).click();
Expand Down Expand Up @@ -107,13 +106,12 @@ describe('dashboard filters list view', () => {
cy.get('[data-test="table-row"]').should('not.exist');
});

xit('should filter by published correctly', () => {
it('should filter by published correctly', () => {
Comment on lines -110 to +109
Copy link
Member

Choose a reason for hiding this comment

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

wow, this was unexpected, bycatch fixing and re-enabling a flaky test 😄

// filter by published
cy.get('.Select__control').eq(2).click();
cy.get('.Select__menu').contains('Published').click();
cy.get('[data-test="table-row"]').should('have.length', 2);
cy.get('[data-test="table-row"]')
.first()
.contains('USA Births Names')
.should('be.visible');
cy.get('.Select__control').eq(2).click();
Expand Down
16 changes: 16 additions & 0 deletions superset-frontend/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions superset-frontend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@
"react-markdown": "^4.3.1",
"react-redux": "^7.2.0",
"react-resize-detector": "^6.0.1-rc.1",
"react-reverse-portal": "^2.0.1",
"react-router-dom": "^5.1.2",
"react-search-input": "^0.11.3",
"react-select": "^3.1.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,21 @@ jest.mock('src/dashboard/actions/dashboardState');

describe('DashboardBuilder', () => {
let favStarStub;
let focusedTabStub;

beforeAll(() => {
// this is invoked on mount, so we stub it instead of making a request
favStarStub = sinon
.stub(dashboardStateActions, 'fetchFaveStar')
.returns({ type: 'mock-action' });
focusedTabStub = sinon
.stub(dashboardStateActions, 'setLastFocusedTab')
.returns({ type: 'mock-action' });
});

afterAll(() => {
favStarStub.restore();
focusedTabStub.restore();
});

function setup(overrideState = {}, overrideStore) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { sliceId } from 'spec/fixtures/mockChartQueries';
import dashboardInfo from 'spec/fixtures/mockDashboardInfo';
import { dashboardLayout as mockLayout } from 'spec/fixtures/mockDashboardLayout';
import { sliceEntitiesForChart } from 'spec/fixtures/mockSliceEntities';
import { nativeFiltersInfo } from '../../fixtures/mockNativeFilters';

describe('ChartHolder', () => {
const props = {
Expand All @@ -55,6 +56,7 @@ describe('ChartHolder', () => {
handleComponentDrop() {},
updateComponents() {},
deleteComponent() {},
nativeFilters: nativeFiltersInfo.filters,
};

function setup(overrideProps) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,10 @@ import HoverMenu from 'src/dashboard/components/menu/HoverMenu';
import DragDroppable from 'src/dashboard/components/dnd/DragDroppable';
import Tabs from 'src/dashboard/components/gridComponents/Tabs';
import { DASHBOARD_ROOT_ID } from 'src/dashboard/util/constants';
import emptyDashboardLayout from 'src/dashboard/fixtures/emptyDashboardLayout';
import { dashboardLayoutWithTabs } from 'spec/fixtures/mockDashboardLayout';
import { mockStoreWithTabs } from 'spec/fixtures/mockStore';
import { nativeFilters } from 'spec/fixtures/mockNativeFilters';

describe('Tabs', () => {
fetchMock.post('glob:*/r/shortner/', {});
Expand All @@ -59,6 +61,8 @@ describe('Tabs', () => {
deleteComponent() {},
updateComponents() {},
logEvent() {},
dashboardLayout: emptyDashboardLayout,
nativeFilters: nativeFilters.filters,
};

function setup(overrideProps) {
Expand Down
5 changes: 5 additions & 0 deletions superset-frontend/src/dashboard/actions/dashboardState.js
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,11 @@ export function setDirectPathToChild(path) {
return { type: SET_DIRECT_PATH, path };
}

export const SET_LAST_FOCUSED_TAB = 'SET_LAST_FOCUSED_TAB';
export function setLastFocusedTab(tabId) {
return { type: SET_LAST_FOCUSED_TAB, tabId };
}

export const SET_FOCUSED_FILTER_FIELD = 'SET_FOCUSED_FILTER_FIELD';
export function setFocusedFilterField(chartId, column) {
return { type: SET_FOCUSED_FILTER_FIELD, chartId, column };
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/dashboard/actions/hydrate.js
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ export const hydrateDashboard = (dashboardData, chartData, datasourcesData) => (
hasUnsavedChanges: false,
maxUndoHistoryExceeded: false,
lastModifiedTime: dashboardData.changed_on,
lastFocusedTabId: null,
},
dashboardLayout,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import { ParentSize } from '@vx/responsive';
import Tabs from 'src/components/Tabs';
import React, { FC, useEffect, useState } from 'react';
import { useSelector } from 'react-redux';
import { useDispatch, useSelector } from 'react-redux';
import DashboardGrid from 'src/dashboard/containers/DashboardGrid';
import getLeafComponentIdFromPath from 'src/dashboard/util/getLeafComponentIdFromPath';
import { DashboardLayout, LayoutItem, RootState } from 'src/dashboard/types';
Expand All @@ -30,6 +30,10 @@ import {
DASHBOARD_ROOT_DEPTH,
} from 'src/dashboard/util/constants';
import { getRootLevelTabIndex } from './utils';
import { Filters } from '../../reducers/types';
import { getChartIdsInFilterScope } from '../../util/activeDashboardFilters';
import { findTabsWithChartsInScope } from '../nativeFilters/utils';
import { setFilterConfiguration } from '../../actions/nativeFilters';

type DashboardContainerProps = {
topLevelTabs?: LayoutItem;
Expand All @@ -39,17 +43,47 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
const dashboardLayout = useSelector<RootState, DashboardLayout>(
state => state.dashboardLayout.present,
);
const nativeFilters = useSelector<RootState, Filters>(
state => state.nativeFilters.filters,
);
const directPathToChild = useSelector<RootState, string[]>(
state => state.dashboardState.directPathToChild,
);
const [tabIndex, setTabIndex] = useState(
getRootLevelTabIndex(dashboardLayout, directPathToChild),
);

const dispatch = useDispatch();

useEffect(() => {
setTabIndex(getRootLevelTabIndex(dashboardLayout, directPathToChild));
}, [getLeafComponentIdFromPath(directPathToChild)]);

// recalculate charts and tabs in scopes of native filters only when a scope or dashboard layout changes
const nativeFiltersValues = Object.values(nativeFilters);
const scopes = nativeFiltersValues.map(filter => filter.scope);
useEffect(() => {
nativeFiltersValues.forEach(filter => {
const filterScope = filter.scope;
const chartsInScope = getChartIdsInFilterScope({
filterScope: {
scope: filterScope.rootPath,
// @ts-ignore
immune: filterScope.excluded,
},
});
const tabsInScope = findTabsWithChartsInScope(
dashboardLayout,
chartsInScope,
);
Object.assign(filter, {
chartsInScope,
tabsInScope: Array.from(tabsInScope),
});
});
dispatch(setFilterConfiguration(nativeFiltersValues));
Copy link

@graceguo-supercat graceguo-supercat Jun 12, 2021

Choose a reason for hiding this comment

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

@kgabryje This update function is called every time a user opens a dashboard, even without any change. This is not a correct behavior.
And, update dashboard should check user permission and get user confirm. This call is secretly changed dashboard json_metadata without user confirm. This will cause serious issue. Please fix asap. Otherwise this PR should be reverted.
cc @junlincc

}, [JSON.stringify(scopes), JSON.stringify(dashboardLayout)]);

const childIds: string[] = topLevelTabs
? topLevelTabs.children
: [DASHBOARD_GRID_ID];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,37 +85,46 @@ const defaultProps = {
* If ChartHolder were a function component, this could be implemented as a hook instead.
*/
const FilterFocusHighlight = React.forwardRef(
({ chartId, focusedFilterScope, ...otherProps }, ref) => {
({ chartId, focusedFilterScope, nativeFilters, ...otherProps }, ref) => {
const theme = useTheme();
if (!focusedFilterScope) return <div ref={ref} {...otherProps} />;
const focusedNativeFilterId = nativeFilters.focusedFilterId;
if (!(focusedFilterScope || focusedNativeFilterId))
return <div ref={ref} {...otherProps} />;

// we use local styles here instead of a conditionally-applied class,
// because adding any conditional class to this container
// causes performance issues in Chrome.

// default to the "de-emphasized" state
let styles = { opacity: 0.3, pointerEvents: 'none' };
const unfocusedChartStyles = { opacity: 0.3, pointerEvents: 'none' };
const focusedChartStyles = {
borderColor: theme.colors.primary.light2,
opacity: 1,
boxShadow: `0px 0px ${theme.gridUnit * 2}px ${
theme.colors.primary.light2
}`,
pointerEvents: 'auto',
};

if (
if (focusedNativeFilterId) {
if (
nativeFilters.filters[focusedNativeFilterId].chartsInScope.includes(
chartId,
)
) {
return <div ref={ref} style={focusedChartStyles} {...otherProps} />;
}
} else if (
chartId === focusedFilterScope.chartId ||
getChartIdsInFilterScope({
filterScope: focusedFilterScope.scope,
}).includes(chartId)
) {
// apply the "highlighted" state if this chart
// contains a filter being focused, or is in scope of a focused filter.
styles = {
borderColor: theme.colors.primary.light2,
opacity: 1,
boxShadow: `0px 0px ${theme.gridUnit * 2}px ${
theme.colors.primary.light2
}`,
pointerEvents: 'auto',
};
return <div ref={ref} style={focusedChartStyles} {...otherProps} />;
}

// inline styles are used here due to a performance issue when adding/changing a class, which causes a reflow
return <div ref={ref} style={styles} {...otherProps} />;
return <div ref={ref} style={unfocusedChartStyles} {...otherProps} />;
},
);

Expand Down Expand Up @@ -233,6 +242,7 @@ class ChartHolder extends React.Component {
isComponentVisible,
dashboardId,
focusedFilterScope,
nativeFilters,
} = this.props;

// inherit the size of parent columns
Expand Down Expand Up @@ -291,6 +301,7 @@ class ChartHolder extends React.Component {
<FilterFocusHighlight
chartId={chartId}
focusedFilterScope={focusedFilterScope}
nativeFilters={nativeFilters}
ref={dragSourceRef}
data-test="dashboard-component-chart-holder"
className={cx(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@
*/

import React from 'react';
import userEvent from '@testing-library/user-event';
import { waitFor } from '@testing-library/react';
import { render, screen } from 'spec/helpers/testing-library';
import mockState from 'spec/fixtures/mockState';
import { sliceId as chartId } from 'spec/fixtures/mockChartQueries';
import { nativeFiltersInfo } from 'spec/javascripts/dashboard/fixtures/mockNativeFilters';
import newComponentFactory from 'src/dashboard/util/newComponentFactory';
import userEvent from '@testing-library/user-event';
import { waitFor } from '@testing-library/react';
import { ChartHolder } from './index';
import { CHART_TYPE, ROW_TYPE } from '../../util/componentTypes';

Expand Down Expand Up @@ -60,6 +61,7 @@ describe('ChartHolder', () => {
editMode: false,
isComponentVisible: true,
dashboardId: 123,
nativeFilters: nativeFiltersInfo.filters,
};

const renderWrapper = (props = defaultProps, state = mockState) =>
Expand Down
Loading