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

chore: Update color scheme when deleted or changed #20589

Merged
merged 40 commits into from
Jul 25, 2022
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
7b6c424
feat(explore): Use v1/explore endpoint data instead of bootstrapData
kgabryje Jun 27, 2022
308745b
Add tests
kgabryje Jun 28, 2022
b47a41f
Fix ci
kgabryje Jun 28, 2022
d687d3e
Remove redundant dependency
kgabryje Jun 28, 2022
c05882a
Use form_data_key in cypress tests
kgabryje Jun 28, 2022
a152491
Add auth headers to for data request
kgabryje Jun 28, 2022
bc3ce1c
Address comments
kgabryje Jun 28, 2022
fe9b97b
Remove displaying danger toast
kgabryje Jun 28, 2022
4e95594
Conditionally add auth headers
kgabryje Jun 28, 2022
d2edbd7
Address comments
kgabryje Jun 29, 2022
7e3e8d7
Fix typing bug
kgabryje Jun 29, 2022
24bc86b
fix
kgabryje Jun 29, 2022
d2e3679
Fix opening dataset
kgabryje Jun 30, 2022
853e707
Merge branch 'feat/explore-v1' of https://github.com/kgabryje/incubat…
geido Jun 30, 2022
d77c547
Fix sqllab chart create
kgabryje Jun 30, 2022
c3f151a
Run queries in parallel
kgabryje Jun 30, 2022
64d9940
Fallback to default color scheme
geido Jun 30, 2022
08ff877
Merge branch 'feat/explore-v1' of https://github.com/kgabryje/incubat…
geido Jun 30, 2022
3c21b3a
Fix dashboard id autofill
kgabryje Jun 30, 2022
1c3f2ef
Fix lint
kgabryje Jun 30, 2022
99a1ebc
Fix test
kgabryje Jun 30, 2022
c2ddb1c
Merge
geido Jun 30, 2022
2f9988b
Fix hydrate action
geido Jun 30, 2022
b8a0837
Update dashboard colors
geido Jul 1, 2022
d941abb
Merge
geido Jul 1, 2022
7bd082e
Add color scheme domain
geido Jul 5, 2022
ef8518f
Add check for default scheme
geido Jul 5, 2022
7e6e9c2
Make me pretty
geido Jul 5, 2022
29e181d
Clean up
geido Jul 7, 2022
6df1dc5
Nit
geido Jul 8, 2022
c39b738
Merge branch 'master' of https://github.com/apache/superset into chor…
geido Jul 8, 2022
2eb74fa
Clean up
geido Jul 8, 2022
4b4529d
Pretty
geido Jul 8, 2022
3b25029
Merge branch 'master' of https://github.com/apache/superset into chor…
geido Jul 11, 2022
1c4e519
Fix missing sequential
geido Jul 11, 2022
2e5869c
Lint
geido Jul 11, 2022
d5f5941
Merge branch 'master' of https://github.com/apache/superset into chor…
geido Jul 14, 2022
fc1d9f6
Enhance test
geido Jul 14, 2022
6af1060
Lint
geido Jul 14, 2022
1518409
Merge
geido Jul 25, 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
1 change: 1 addition & 0 deletions superset-frontend/src/dashboard/actions/dashboardState.js
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ export function saveDashboardRequest(data, id, saveType) {
...data.metadata,
color_namespace: data.metadata?.color_namespace || undefined,
color_scheme: data.metadata?.color_scheme || '',
color_scheme_domain: data.metadata?.color_scheme_domain || [],
expanded_slices: data.metadata?.expanded_slices || {},
label_colors: data.metadata?.label_colors || {},
shared_label_colors: data.metadata?.shared_label_colors || {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,23 @@
*/
// ParentSize uses resize observer so the dashboard will update size
// when its container size changes, due to e.g., builder side panel opening
import React, { FC, useEffect, useMemo } from 'react';
import React, { FC, useCallback, useEffect, useMemo } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import {
FeatureFlag,
Filter,
Filters,
getCategoricalSchemeRegistry,
isFeatureEnabled,
SupersetClient,
} from '@superset-ui/core';
import { ParentSize } from '@vx/responsive';
import pick from 'lodash/pick';
import Tabs from 'src/components/Tabs';
import DashboardGrid from 'src/dashboard/containers/DashboardGrid';
import {
ChartsState,
DashboardInfo,
DashboardLayout,
LayoutItem,
RootState,
Expand All @@ -43,9 +46,13 @@ import {
import { getChartIdsInFilterScope } from 'src/dashboard/util/getChartIdsInFilterScope';
import findTabIndexByComponentId from 'src/dashboard/util/findTabIndexByComponentId';
import { setInScopeStatusOfFilters } from 'src/dashboard/actions/nativeFilters';
import { getRootLevelTabIndex, getRootLevelTabsComponent } from './utils';
import { findTabsWithChartsInScope } from '../nativeFilters/utils';
import { dashboardInfoChanged } from 'src/dashboard/actions/dashboardInfo';
import { setColorScheme } from 'src/dashboard/actions/dashboardState';
import { useComponentDidUpdate } from 'src/hooks/useComponentDidUpdate/useComponentDidUpdate';
import jsonStringify from 'json-stringify-pretty-compact';
import { NATIVE_FILTER_DIVIDER_PREFIX } from '../nativeFilters/FiltersConfigModal/utils';
import { findTabsWithChartsInScope } from '../nativeFilters/utils';
import { getRootLevelTabIndex, getRootLevelTabsComponent } from './utils';

type DashboardContainerProps = {
topLevelTabs?: LayoutItem;
Expand Down Expand Up @@ -73,6 +80,9 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
const dashboardLayout = useSelector<RootState, DashboardLayout>(
state => state.dashboardLayout.present,
);
const dashboardInfo = useSelector<RootState, DashboardInfo>(
state => state.dashboardInfo,
);
const directPathToChild = useSelector<RootState, string[]>(
state => state.dashboardState.directPathToChild,
);
Expand Down Expand Up @@ -122,10 +132,86 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
dispatch(setInScopeStatusOfFilters(scopes));
}, [nativeFilterScopes, dashboardLayout, dispatch]);

const verifyUpdateColorScheme = useCallback(() => {
const currentMetadata = dashboardInfo.metadata;
if (currentMetadata?.color_scheme) {
const metadata = { ...currentMetadata };
const colorScheme = metadata?.color_scheme;
const colorSchemeDomain = metadata?.color_scheme_domain || [];
const categoricalSchemes = getCategoricalSchemeRegistry();
const registryColorSchemeDomain =
categoricalSchemes.get(colorScheme)?.colors || [];
const defaultColorScheme = categoricalSchemes.defaultKey;
const isColorSchemeExisting = !!categoricalSchemes.get(colorScheme);
geido marked this conversation as resolved.
Show resolved Hide resolved
const updateDashboard = () => {
SupersetClient.put({
endpoint: `/api/v1/dashboard/${dashboardInfo.id}`,
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
json_metadata: jsonStringify(metadata),
}),
}).catch(e => console.log(e));
geido marked this conversation as resolved.
Show resolved Hide resolved
};
const updateColorScheme = (scheme: string) => {
dispatch(setColorScheme(scheme));
};
const updateMetadata = () => {
dispatch(
dashboardInfoChanged({
metadata,
}),
);
updateDashboard();
};
// color scheme does not exist anymore
// must fallback to the available default one
if (colorScheme && !isColorSchemeExisting) {
const updatedScheme =
defaultColorScheme?.toString() || 'supersetColors';
metadata.color_scheme = updatedScheme;
metadata.color_scheme_domain =
categoricalSchemes.get(defaultColorScheme)?.colors || [];

// reset shared_label_colors
// TODO: Requires regenerating the shared_label_colors after
// fixing a bug which affects their generation on dashboards with tabs
metadata.shared_label_colors = {};

updateColorScheme(updatedScheme);
updateMetadata();
} else {
// if this dashboard does not have a color_scheme_domain saved
// must create one and store it for the first time
if (colorScheme && !colorSchemeDomain.length) {
metadata.color_scheme_domain = registryColorSchemeDomain;
updateMetadata();
}
// if the color_scheme_domain is not the same as the registry domain
// must update the existing color_scheme_domain
if (
colorScheme &&
colorSchemeDomain.length &&
registryColorSchemeDomain.toString() !== colorSchemeDomain.toString()
) {
metadata.color_scheme_domain = registryColorSchemeDomain;

// reset shared_label_colors
// TODO: Requires regenerating the shared_label_colors after
// fixing a bug which affects their generation on dashboards with tabs
metadata.shared_label_colors = {};

updateColorScheme(colorScheme);
updateMetadata();
}
}
}
}, [charts]);

useComponentDidUpdate(verifyUpdateColorScheme);

const childIds: string[] = topLevelTabs
? topLevelTabs.children
: [DASHBOARD_GRID_ID];

const min = Math.min(tabIndex, childIds.length - 1);
const activeKey = min === 0 ? DASHBOARD_GRID_ID : min.toString();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ test('submitting with onlyApply:false', async () => {
);
spyGetCategoricalSchemeRegistry.mockReturnValue({
keys: () => ['supersetColors'],
get: () => ['#FFFFFF', '#000000'],
} as any);
put.mockResolvedValue({
json: {
Expand Down Expand Up @@ -289,7 +290,6 @@ test('submitting with onlyApply:false', async () => {

userEvent.click(screen.getByRole('button', { name: 'Save' }));
await waitFor(() => {
expect(props.onHide).toBeCalledTimes(1);
expect(props.onSubmit).toBeCalledTimes(1);
expect(props.onSubmit).toBeCalledWith({
certificationDetails: 'Sample certification',
Expand All @@ -312,6 +312,7 @@ test('submitting with onlyApply:true', async () => {
);
spyGetCategoricalSchemeRegistry.mockReturnValue({
keys: () => ['supersetColors'],
get: () => ['#FFFFFF', '#000000'],
} as any);
spyIsFeatureEnabled.mockReturnValue(false);
const props = createProps();
Expand All @@ -328,7 +329,6 @@ test('submitting with onlyApply:true', async () => {

userEvent.click(screen.getByRole('button', { name: 'Apply' }));
await waitFor(() => {
expect(props.onHide).toBeCalledTimes(1);
expect(props.onSubmit).toBeCalledTimes(1);
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,13 @@ const PropertiesModal = ({
delete metadata.positions;
}
const metaDataCopy = { ...metadata };

if (metaDataCopy?.shared_label_colors) {
delete metaDataCopy.shared_label_colors;
}
if (metaDataCopy?.color_scheme_domain) {
geido marked this conversation as resolved.
Show resolved Hide resolved
delete metaDataCopy.color_scheme_domain;
}
setJsonMetadata(metaDataCopy ? jsonStringify(metaDataCopy) : '');
},
[form],
Expand Down Expand Up @@ -262,7 +266,8 @@ const PropertiesModal = ({
{ updateMetadata = true } = {},
) => {
// check that color_scheme is valid
const colorChoices = getCategoricalSchemeRegistry().keys();
const categoricalSchemeRegistry = getCategoricalSchemeRegistry();
const colorChoices = categoricalSchemeRegistry.keys();
const jsonMetadataObj = getJsonMetadata();

// only fire if the color_scheme is present and invalid
Expand All @@ -288,6 +293,7 @@ const PropertiesModal = ({
const onFinish = () => {
const { title, slug, certifiedBy, certificationDetails } =
form.getFieldsValue();
const categoricalSchemeRegistry = getCategoricalSchemeRegistry();
geido marked this conversation as resolved.
Show resolved Hide resolved
let currentColorScheme = colorScheme;
let colorNamespace = '';
let currentJsonMetadata = jsonMetadata;
Expand All @@ -302,12 +308,17 @@ const PropertiesModal = ({
if (metadata?.shared_label_colors) {
delete metadata.shared_label_colors;
}
if (metadata?.color_scheme_domain) {
delete metadata.color_scheme_domain;
}
const colorMap = getSharedLabelColor().getColorMap(
colorNamespace,
currentColorScheme,
true,
);
metadata.shared_label_colors = colorMap;
metadata.color_scheme_domain =
categoricalSchemeRegistry.get(colorScheme)?.colors || [];
currentJsonMetadata = jsonStringify(metadata);
}

Expand Down
4 changes: 4 additions & 0 deletions superset-frontend/src/dashboard/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ export type DashboardInfo = {
native_filter_configuration: JsonObject;
show_native_filters: boolean;
chart_configuration: JsonObject;
color_scheme: string;
color_namespace: string;
color_scheme_domain: string[];
shared_label_colors: Record<string, string>;
};
};

Expand Down
31 changes: 30 additions & 1 deletion superset-frontend/src/explore/actions/hydrateExplore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@ import {
import { getChartKey } from 'src/explore/exploreUtils';
import { getControlsState } from 'src/explore/store';
import { Dispatch } from 'redux';
import { ensureIsArray } from '@superset-ui/core';
import {
ensureIsArray,
getCategoricalSchemeRegistry,
getSequentialSchemeRegistry,
} from '@superset-ui/core';
import {
getFormDataFromControls,
applyMapStateToPropsToControl,
Expand Down Expand Up @@ -67,6 +71,31 @@ export const hydrateExplore =
initialExploreState,
initialFormData,
) as ControlStateMapping;
const colorSchemeKey = initialControls.color_scheme && 'color_scheme';
const linearColorSchemeKey =
initialControls.linear_color_scheme && 'linear_color_scheme';
// if the color scheme does not exist anymore
// fallbacks to the available default key
const verifyColorScheme = (type: 'CATEGORICAL' | 'SEQUENTIAL') => {
geido marked this conversation as resolved.
Show resolved Hide resolved
const schemes =
type === 'CATEGORICAL'
? getCategoricalSchemeRegistry()
: getSequentialSchemeRegistry();
const key =
type === 'CATEGORICAL' ? colorSchemeKey : linearColorSchemeKey;
const defaultScheme = schemes.defaultKey
? schemes.defaultKey
: type === 'CATEGORICAL'
? 'supersetColors'
: 'superset_seq_1';
geido marked this conversation as resolved.
Show resolved Hide resolved
const currentScheme = initialFormData[key];
if (currentScheme && !schemes.get(currentScheme)) {
initialControls[key].value = defaultScheme;
}
};

if (colorSchemeKey) verifyColorScheme('CATEGORICAL');
if (linearColorSchemeKey) verifyColorScheme('SEQUENTIAL');

const exploreState = {
// note this will add `form_data` to state,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ export default class ColorSchemeControl extends React.PureComponent {
onChange: this.onChange,
options,
placeholder: t('Select scheme'),
value: currentScheme,
value: currentScheme && currentScheme.toString(),
geido marked this conversation as resolved.
Show resolved Hide resolved
};

return (
Expand Down
2 changes: 1 addition & 1 deletion superset/dashboards/dao.py
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ def set_dash_metadata( # pylint: disable=too-many-locals
md["color_scheme"] = data.get("color_scheme", "")
md["label_colors"] = data.get("label_colors", {})
md["shared_label_colors"] = data.get("shared_label_colors", {})

md["color_scheme_domain"] = data.get("color_scheme_domain", [])
dashboard.json_metadata = json.dumps(md)

if commit:
Expand Down
1 change: 1 addition & 0 deletions superset/dashboards/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ class DashboardJSONMetadataSchema(Schema):
positions = fields.Dict(allow_none=True)
label_colors = fields.Dict()
shared_label_colors = fields.Dict()
color_scheme_domain = fields.List(fields.Str())
# used for v0 import/export
import_time = fields.Integer()
remote_id = fields.Integer()
Expand Down
2 changes: 1 addition & 1 deletion tests/integration_tests/dashboards/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class TestDashboardApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixi
"slug": "slug1_changed",
"position_json": '{"b": "B"}',
"css": "css_changed",
"json_metadata": '{"refresh_frequency": 30, "timed_refresh_immune_slices": [], "expanded_slices": {}, "color_scheme": "", "label_colors": {}, "shared_label_colors": {}}',
"json_metadata": '{"refresh_frequency": 30, "timed_refresh_immune_slices": [], "expanded_slices": {}, "color_scheme": "", "label_colors": {}, "shared_label_colors": {}, "color_scheme_domain": []}',
"published": False,
}

Expand Down