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

Jump to bottom

refactor(dashboard): [chart-maximize-mode]put chart full-size state in redux #15384

Merged
merged 2 commits into from Jul 2, 2021
Merged

refactor(dashboard): [chart-maximize-mode]put chart full-size state in redux #15384

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
2403900
refactor(dashboard): [chart-maximize-mode]put full-size status to redux
stephenLYZ Jun 25, 2021
9d8e873
fix: ci
stephenLYZ Jun 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions superset-frontend/src/dashboard/actions/dashboardState.js
View file
Open in desktop
Expand Up @@ -359,6 +359,11 @@ export function unsetFocusedFilterField(chartId, column) {
return { type: UNSET_FOCUSED_FILTER_FIELD, chartId, column };
}

export const SET_FULL_SIZE_CHART_ID = 'SET_FULL_SIZE_CHART_ID';
export function setFullSizeChartId(chartId) {
return { type: SET_FULL_SIZE_CHART_ID, chartId };
}

// Undo history ---------------------------------------------------------------
export const SET_MAX_UNDO_HISTORY_EXCEEDED = 'SET_MAX_UNDO_HISTORY_EXCEEDED';
export function setMaxUndoHistoryExceeded(maxUndoHistoryExceeded = true) {
Expand Down
11 changes: 8 additions & 3 deletions superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
View file
Open in desktop
Expand Up @@ -90,10 +90,12 @@ const StyledHeader = styled.div`
z-index: 2;
`;

const StyledContent = styled.div`
const StyledContent = styled.div<{
fullSizeChartId: number | null;
}>`
grid-column: 2;
grid-row: 2;
z-index: 1;
z-index: ${({ fullSizeChartId }) => (fullSizeChartId ? 1000 : 1)};
Copy link
Contributor

@simcha90 simcha90 Jun 29, 2021 •
edited

Choose a reason for hiding this comment

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

I think z-index: 2 also enough, I think better not to use big zIndexes because if need later override them it will be more difficult

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to make a place to store z-index values as constants so that they are all collected in one place and can be adjusted and compared easily. Maybe this PR isn't the place to do that though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agreed. There seems to be no place to manage all z-index values.

`;

const StyledDashboardContent = styled.div<{
Expand Down Expand Up @@ -147,6 +149,9 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
const directPathToChild = useSelector<RootState, string[]>(
state => state.dashboardState.directPathToChild,
);
const fullSizeChartId = useSelector<RootState, number | null>(
state => state.dashboardState.fullSizeChartId,
);

const handleChangeTab = ({
pathToTabIndex,
Expand Down Expand Up @@ -276,7 +281,7 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => {
)}
</DragDroppable>
</StyledHeader>
<StyledContent>
<StyledContent fullSizeChartId={fullSizeChartId}>
<div
data-test="dashboard-content"
className={cx('dashboard', editMode && 'dashboard--editing')}
Expand Down
20 changes: 13 additions & 7 deletions superset-frontend/src/dashboard/components/gridComponents/ChartHolder.jsx
View file
Open in desktop
Expand Up @@ -54,6 +54,7 @@ const propTypes = {
directPathToChild: PropTypes.arrayOf(PropTypes.string),
directPathLastUpdated: PropTypes.number,
focusedFilterScope: PropTypes.object,
fullSizeChartId: PropTypes.oneOf([PropTypes.number, null]),

// grid related
availableColumnCount: PropTypes.number.isRequired,
Expand All @@ -66,6 +67,7 @@ const propTypes = {
deleteComponent: PropTypes.func.isRequired,
updateComponents: PropTypes.func.isRequired,
handleComponentDrop: PropTypes.func.isRequired,
setFullSizeChartId: PropTypes.func.isRequired,
};

const defaultProps = {
Expand Down Expand Up @@ -189,7 +191,6 @@ class ChartHolder extends React.Component {
outlinedComponentId: null,
outlinedColumnName: null,
directPathLastUpdated: 0,
isFullSize: false,
};

this.handleChangeFocus = this.handleChangeFocus.bind(this);
Expand Down Expand Up @@ -244,7 +245,10 @@ class ChartHolder extends React.Component {
}

handleToggleFullSize() {
this.setState(prevState => ({ isFullSize: !prevState.isFullSize }));
const { component, fullSizeChartId, setFullSizeChartId } = this.props;
const { chartId } = component.meta;
const isFullSize = fullSizeChartId === chartId;
setFullSizeChartId(isFullSize ? null : chartId);
}

render() {
Expand All @@ -263,8 +267,12 @@ class ChartHolder extends React.Component {
editMode,
isComponentVisible,
dashboardId,
fullSizeChartId,
} = this.props;

const { chartId } = component.meta;
const isFullSize = fullSizeChartId === chartId;

// inherit the size of parent columns
const widthMultiple =
parentComponent.type === COLUMN_TYPE
Expand All @@ -274,7 +282,7 @@ class ChartHolder extends React.Component {
let chartWidth = 0;
let chartHeight = 0;

if (this.state.isFullSize) {
if (isFullSize) {
chartWidth = window.innerWidth - CHART_MARGIN;
chartHeight = window.innerHeight - CHART_MARGIN;
} else {
Expand All @@ -288,8 +296,6 @@ class ChartHolder extends React.Component {
);
}

const { chartId } = component.meta;

return (
<DragDroppable
component={component}
Expand Down Expand Up @@ -326,7 +332,7 @@ class ChartHolder extends React.Component {
'dashboard-component',
'dashboard-component-chart-holder',
this.state.outlinedComponentId ? 'fade-in' : 'fade-out',
this.state.isFullSize && 'full-size',
isFullSize && 'full-size',
)}
>
{!editMode && (
Expand All @@ -351,7 +357,7 @@ class ChartHolder extends React.Component {
updateSliceName={this.handleUpdateSliceName}
isComponentVisible={isComponentVisible}
handleToggleFullSize={this.handleToggleFullSize}
isFullSize={this.state.isFullSize}
isFullSize={isFullSize}
/>
{editMode && (
<HoverMenu position="top">
Expand Down
14 changes: 4 additions & 10 deletions superset-frontend/src/dashboard/components/gridComponents/ChartHolder.test.tsx
View file
Open in desktop
Expand Up @@ -18,7 +18,6 @@
*/

import React from 'react';
import userEvent from '@testing-library/user-event';
import { waitFor } from '@testing-library/react';
import { sliceId as chartId } from 'spec/fixtures/mockChartQueries';
import { nativeFiltersInfo } from 'spec/javascripts/dashboard/fixtures/mockNativeFilters';
Expand Down Expand Up @@ -66,6 +65,8 @@ describe('ChartHolder', () => {
isComponentVisible: true,
dashboardId: 123,
nativeFilters: nativeFiltersInfo.filters,
fullSizeChartId: chartId,
setFullSizeChartId: () => {},
};
const mockStore = getMockStore({
...initialState,
Expand All @@ -79,19 +80,12 @@ describe('ChartHolder', () => {
</Provider>,
);

it('toggle full size', async () => {
it('should render full size', async () => {
renderWrapper();

let chart = (screen.getByTestId('slice-container')
const chart = (screen.getByTestId('slice-container')
.firstChild as HTMLElement).style;
expect(chart?.width).toBe('900px');
expect(chart?.height).toBe('26px');

userEvent.click(screen.getByRole('button'));
userEvent.click(screen.getByText('Maximize chart'));

chart = (screen.getByTestId('slice-container').firstChild as HTMLElement)
.style;
await waitFor(() => expect(chart?.width).toBe('992px'));
expect(chart?.height).toBe('714px');
});
Expand Down
8 changes: 7 additions & 1 deletion superset-frontend/src/dashboard/containers/DashboardComponent.jsx
View file
Open in desktop
Expand Up @@ -35,7 +35,11 @@ import {
updateComponents,
handleComponentDrop,
} from '../actions/dashboardLayout';
import { setDirectPathToChild, setActiveTabs } from '../actions/dashboardState';
import {
setDirectPathToChild,
setActiveTabs,
setFullSizeChartId,
} from '../actions/dashboardState';

const propTypes = {
id: PropTypes.string,
Expand Down Expand Up @@ -83,6 +87,7 @@ function mapStateToProps(
activeTabs: dashboardState.activeTabs,
directPathLastUpdated: dashboardState.directPathLastUpdated,
dashboardId: dashboardInfo.id,
fullSizeChartId: dashboardState.fullSizeChartId,
};

// rows and columns need more data about their child dimensions
Expand Down Expand Up @@ -112,6 +117,7 @@ function mapDispatchToProps(dispatch) {
updateComponents,
handleComponentDrop,
setDirectPathToChild,
setFullSizeChartId,
setActiveTabs,
logEvent,
},
Expand Down
7 changes: 7 additions & 0 deletions superset-frontend/src/dashboard/reducers/dashboardState.js
View file
Open in desktop
Expand Up @@ -36,6 +36,7 @@ import {
SET_FOCUSED_FILTER_FIELD,
UNSET_FOCUSED_FILTER_FIELD,
SET_ACTIVE_TABS,
SET_FULL_SIZE_CHART_ID,
} from '../actions/dashboardState';
import { HYDRATE_DASHBOARD } from '../actions/hydrate';

Expand Down Expand Up @@ -165,6 +166,12 @@ export default function dashboardStateReducer(state = {}, action) {
focusedFilterField: null,
};
},
[SET_FULL_SIZE_CHART_ID]() {
return {
...state,
fullSizeChartId: action.chartId,
};
},
};

if (action.type in actionHandlers) {
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/dashboard/types.ts
View file
Open in desktop
Expand Up @@ -56,6 +56,7 @@ export type DashboardState = {
editMode: boolean;
directPathToChild: string[];
activeTabs: ActiveTabs;
fullSizeChartId: number | null;
};
export type DashboardInfo = {
common: {
Expand Down