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

fix: should be able to remove selection from X-AXIS control #21371

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

zhaoyongjie
Copy link
Member

@zhaoyongjie zhaoyongjie commented Sep 8, 2022

SUMMARY

Should be able to remove a column from X-AXIS control, since the x-axis control has supported categorical or date columns.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

remove.axis.before.mov

After

should.be.able.to.remove.selection.column.from.x-axis.mov

TESTING INSTRUCTIONS

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

@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #21371 (3bec298) into master (9c285da) will increase coverage by 0.03%.
The diff coverage is 35.71%.

❗ Current head 3bec298 differs from pull request most recent head e1098bb. Consider uploading reports for the commit e1098bb to get more accurate results

@@            Coverage Diff             @@
##           master   #21371      +/-   ##
==========================================
+ Coverage   66.53%   66.57%   +0.03%     
==========================================
  Files        1791     1791              
  Lines       68599    68559      -40     
  Branches     7320     7319       -1     
==========================================
- Hits        45645    45644       -1     
+ Misses      21064    21021      -43     
- Partials     1890     1894       +4     
Flag Coverage Δ
javascript 52.73% <35.71%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...d/packages/superset-ui-chart-controls/src/types.ts 100.00% <ø> (ø)
...ontend/src/explore/controlUtils/getControlState.ts 80.32% <28.57%> (-10.42%) ⬇️
...i-chart-controls/src/shared-controls/constants.tsx 33.33% <42.85%> (+14.58%) ⬆️
.../explore/components/controls/TextControl/index.tsx 76.66% <0.00%> (-10.00%) ⬇️
...ntend/src/explore/components/ExploreChartPanel.jsx 67.60% <0.00%> (-4.02%) ⬇️
...plore/components/DataTablesPane/DataTablesPane.tsx 78.04% <0.00%> (-2.44%) ⬇️
superset/explore/commands/get.py 89.65% <0.00%> (-2.30%) ⬇️
.../src/explore/components/controls/SelectControl.jsx 62.50% <0.00%> (-1.79%) ⬇️
...ontrols/AnnotationLayerControl/AnnotationLayer.jsx 74.25% <0.00%> (-0.60%) ⬇️
... and 15 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

I left a few comments. I think the important thing to test is to ensure that a chart that has been created before the flag was enabled still renders correctly (=with the x-axis control being set to the temporal column)

Comment on lines -42 to -53
// default to the chosen time column if x-axis is unset and the
// GENERIC_CHART_AXES feature flag is enabled
const { value } = control;
if (value) {
return value;
}
const timeColumn = controlPanel?.form_data?.granularity_sqla;
if (isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && timeColumn) {
return timeColumn;
}
return null;
},
Copy link
Member

@villebro villebro Sep 8, 2022

Choose a reason for hiding this comment

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

IIRC, this logic was added so that charts that were created when the GENERIC_CHART_AXES FF was unset would render correctly when exploring them after the GENERIC_CHART_AXES FF was enabled. Otherwise it would open Explor and clear the chart due to the required x_axis control being unset. So the flow should be something like this:

  • When entering Explore with FF disabled, nothing should happen (x_axis should be null)
  • When entering Explore with the FF enabled and the x_axis value is unset, the value should be set to the temporal column
  • When entering Explore with the FF enabled and the x_axis value is set, it should not be changed

Copy link
Member Author

Choose a reason for hiding this comment

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

The X-Axis control value should be set instead of setting the default value because if the default value had been set, the X-Axis control wouldn't have chance to remove it(the X-AXIS always sets a default value). Let me find a way to set the X-Axis as the sqla_grainularity if GENERIC_CHART_AXES is enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @villebro, I have added a new property for controls, initialValue. It refers to how to initialize a control when loading, this setting avoids the default value always set to the control.

I also tested charts between FF enabled and disabled.

  1. Create a Bar Chart V2 when GENERIC_CHART_AXES is disabled, and then create a dashboard and take the chart.
  2. Open the GENERIC_CHART_AXES
  3. the Chart on the Explore page and Dashboard same as the disabled FF.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @villebro, I have added a new property for controls, initialValue. It refers to how to initialize a control when loading, this setting avoids the default value always set to the control.

This is a really useful addition 👍 I'm sure this will be needed in the future at some point.

@zhaoyongjie zhaoyongjie changed the title fix: should be able to remove selection from X-AXIS control [WIP]fix: should be able to remove selection from X-AXIS control Sep 9, 2022
@zhaoyongjie zhaoyongjie changed the title [WIP]fix: should be able to remove selection from X-AXIS control fix: should be able to remove selection from X-AXIS control Sep 9, 2022
@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 11, 2022
@zhaoyongjie
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

@zhaoyongjie Ephemeral environment spinning up at http://34.212.24.2:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM and tested to work as expected 👍

Comment on lines -42 to -53
// default to the chosen time column if x-axis is unset and the
// GENERIC_CHART_AXES feature flag is enabled
const { value } = control;
if (value) {
return value;
}
const timeColumn = controlPanel?.form_data?.granularity_sqla;
if (isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && timeColumn) {
return timeColumn;
}
return null;
},
Copy link
Member

Choose a reason for hiding this comment

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

Hi @villebro, I have added a new property for controls, initialValue. It refers to how to initialize a control when loading, this setting avoids the default value always set to the control.

This is a really useful addition 👍 I'm sure this will be needed in the future at some point.

@zhaoyongjie zhaoyongjie merged commit eb4ba5b into apache:master Sep 13, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants