fix(explore): hide advanced analytics for non temporal xaxis #28312

justinpark · 2024-05-01T23:19:26Z

SUMMARY

The superset feature allows users to choose a non-temporal column for the x-axis.
However, when a non-temporal column is selected, advanced analytics and forecast options are not compatible.

In order to address this, UI should dynamically hide these features when a non-temporal column is selected.
This commit introduces the visibility option in the ControlPanelSectionConfig, enabling the hiding of specific groups of controlSets.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

before--hide-non-temporal.mov

After:

after--hide-non-temporal.mov

TESTING INSTRUCTIONS

Choose non-temporal column in x-axis option
Verify that "Advanced analytics" and "Predictive Analytics" sections are hidden

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

michael-s-molina

Thank you for the PR @justinpark. I left some first-pass comments.

michael-s-molina · 2024-05-02T11:38:46Z

superset-frontend/packages/superset-ui-chart-controls/src/utils/showIfTimeSeries.ts

+import { isAdhocColumn, isPhysicalColumn } from '@superset-ui/core';
+import type { ColumnMeta, ControlPanelsContainerProps } from '../types';
+
+export default function showIfTimeSeries({


Can we rename this to showTimeRelatedControls?

@michael-s-molina to clarify and specify the functionality, i'm thinking of use displayWhenTimeRelatedControls.
What do you think?

displayTimeRelatedControls (without when) works too.

michael-s-molina · 2024-05-02T11:40:05Z

superset-frontend/packages/superset-ui-chart-controls/test/utils/showIfTimeSeries.test.ts

+  },
+};
+
+describe('showIfTimeSeries', () => {


Can you avoid describe?

michael-s-molina · 2024-05-02T11:44:53Z

superset-frontend/src/explore/components/ControlPanelsContainer.test.tsx

+      controlPanelSections: [
+        {
+          label: t('Advanced analytics'),
+          description: t('Advanced analytical post processin'),


Suggested change

description: t('Advanced analytical post processin'),

description: t('Advanced analytics post processing'),

michael-s-molina · 2024-05-02T11:48:33Z

superset-frontend/src/explore/components/ControlPanelsContainer.test.tsx

@@ -144,4 +144,41 @@ describe('ControlPanelsContainer', () => {
      await screen.findAllByTestId('collapsible-control-panel-header'),
    ).toHaveLength(2);
  });
+
+  test('hide ControlPanelSections when its visibility is false', async () => {


You're also testing if the panel is displayed when the visibility is true.

Suggested change

test('hide ControlPanelSections when its visibility is false', async () => {

test('visibility of panels is correctly applied', async () => {

michael-s-molina · 2024-05-02T17:22:54Z

@justinpark Can you also make sure to ignore or remove the values from the form data in case a user starts with a temporal column, fills the advanced properties and later changes to a categorial x-axis?

justinpark · 2024-05-08T00:07:06Z

@justinpark Can you also make sure to ignore or remove the values from the form data in case a user starts with a temporal column, fills the advanced properties and later changes to a categorial x-axis?

Done

(cherry picked from commit 07cd1d8)

…28312)

Vovch · 2024-05-16T14:47:55Z

superset-frontend/src/explore/reducers/exploreReducer.js

+          ...form_data,
+          ...restoredField,
+        },
+        hiddenFormData,


A bit too late, but I guess there should be omit(hiddenFormData, fieldNames), otherwise it seems to break any field that was hidden once.

@Vovch thanks for catching up. I'll post the hotfix which will fix the #29331

…28312)

sfirke · 2024-06-21T19:32:49Z

@Vovch are you saying this introduced a bug? Based on your comment I am trying this out in 4.0.2rc1 and indeed experiencing something that might be a bug. I created a new issue #29331 for it and would appreciate if you can expand on whether this is what you were talking about.

…28312)

pull-request-size bot added the size/L label May 1, 2024

justinpark requested a review from michael-s-molina May 1, 2024 23:19

github-actions bot added the packages label May 1, 2024

justinpark requested review from betodealmeida, john-bodley and eschutho May 1, 2024 23:19

justinpark changed the title ~~Fix explore hide advanced analytics for non temporal xaxis~~ fix(explore): hide advanced analytics for non temporal xaxis May 2, 2024

michael-s-molina reviewed May 2, 2024

View reviewed changes

michael-s-molina requested a review from geido May 2, 2024 11:50

justinpark force-pushed the fix--explore-hide-advanced-analytics-for-non-temporal-xaxis branch from d5e2c33 to 24da739 Compare May 2, 2024 17:16

pull-request-size bot added size/XL and removed size/L labels May 7, 2024

justinpark force-pushed the fix--explore-hide-advanced-analytics-for-non-temporal-xaxis branch from 800e989 to f1278a8 Compare May 7, 2024 23:23

justinpark added 5 commits May 8, 2024 09:25

fix(explore): Hide advance analytics for non time-series

3f8ed15

hide forecast for non-temporal too

4407517

rename function name

d0776f6

add spec for non physical column

cb6b989

stash form_data on invisible

c213433

justinpark force-pushed the fix--explore-hide-advanced-analytics-for-non-temporal-xaxis branch from f1278a8 to c213433 Compare May 8, 2024 16:25

missing spec

4e26d5d

john-bodley requested a review from michael-s-molina May 8, 2024 20:05

michael-s-molina approved these changes May 8, 2024

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label May 8, 2024

michael-s-molina merged commit 07cd1d8 into apache:master May 8, 2024
29 checks passed

michael-s-molina added the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label May 8, 2024

michael-s-molina pushed a commit that referenced this pull request May 8, 2024

fix(explore): hide advanced analytics for non temporal xaxis (#28312)

17b5367

(cherry picked from commit 07cd1d8)

imancrsrk pushed a commit to imancrsrk/superset that referenced this pull request May 10, 2024

fix(explore): hide advanced analytics for non temporal xaxis (apache#…

412927c

…28312)

jzhao62 pushed a commit to jzhao62/superset that referenced this pull request May 16, 2024

fix(explore): hide advanced analytics for non temporal xaxis (apache#…

8c79130

…28312)

Vovch reviewed May 16, 2024

View reviewed changes

EnxDev pushed a commit to EnxDev/superset that referenced this pull request May 31, 2024

fix(explore): hide advanced analytics for non temporal xaxis (apache#…

a666e04

…28312)

sfirke mentioned this pull request Jun 21, 2024

Changing to non-temporal variable, then back, causes time grain to no longer function in 4.0.2rc2 #29331

Closed

3 tasks

kgabryje mentioned this pull request Jul 9, 2024

fix: Table time comparison breaking after form data update #29525

Merged

9 tasks

mistercrunch added 🍒 4.0.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Jul 24, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

fix(explore): hide advanced analytics for non temporal xaxis (apache#…

0aa4c61

…28312)

qmb-jschwartz mentioned this pull request Nov 22, 2024

Advanced Analytics hidden for Non-Timeseries Data #31027

Open

3 tasks

mistercrunch added the 🚢 4.1.0 label Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(explore): hide advanced analytics for non temporal xaxis #28312

fix(explore): hide advanced analytics for non temporal xaxis #28312

justinpark commented May 1, 2024

michael-s-molina left a comment

michael-s-molina May 2, 2024

justinpark May 2, 2024

michael-s-molina May 2, 2024 •

edited

Loading

michael-s-molina May 2, 2024

michael-s-molina May 2, 2024

michael-s-molina May 2, 2024

michael-s-molina commented May 2, 2024

justinpark commented May 8, 2024

Vovch May 16, 2024

justinpark Jun 24, 2024

sfirke commented Jun 21, 2024

	description: t('Advanced analytical post processin'),
	description: t('Advanced analytics post processing'),

	test('hide ControlPanelSections when its visibility is false', async () => {
	test('visibility of panels is correctly applied', async () => {

fix(explore): hide advanced analytics for non temporal xaxis #28312

fix(explore): hide advanced analytics for non temporal xaxis #28312

Conversation

justinpark commented May 1, 2024

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

michael-s-molina left a comment

Choose a reason for hiding this comment

michael-s-molina May 2, 2024

Choose a reason for hiding this comment

justinpark May 2, 2024

Choose a reason for hiding this comment

michael-s-molina May 2, 2024 • edited Loading

Choose a reason for hiding this comment

michael-s-molina May 2, 2024

Choose a reason for hiding this comment

michael-s-molina May 2, 2024

Choose a reason for hiding this comment

michael-s-molina May 2, 2024

Choose a reason for hiding this comment

michael-s-molina commented May 2, 2024

justinpark commented May 8, 2024

Vovch May 16, 2024

Choose a reason for hiding this comment

justinpark Jun 24, 2024

Choose a reason for hiding this comment

sfirke commented Jun 21, 2024

michael-s-molina May 2, 2024 •

edited

Loading