You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2023/10/06 14:31:59 UTC
[PR] feat: Improves the Waterfall chart [superset]
michael-s-molina opened a new pull request, #25557:
URL: https://github.com/apache/superset/pull/25557
### SUMMARY
This PR is a follow-up of https://github.com/apache/superset/pull/17906 and does the following improvements:
- Improves the chart description
- Improves the support for temporal columns the Dimension control
- Fixes a bug with negative values where the bars are displayed above the 0 line
Hat tip to @rusackas for testing the plugin and documenting the issues [here](https://github.com/apache/superset/pull/17906#issuecomment-1749740928).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TODO
### TESTING INSTRUCTIONS
TODO
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Improves the Waterfall chart [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25557:
URL: https://github.com/apache/superset/pull/25557#issuecomment-1792747894
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Improves the Waterfall chart [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #25557:
URL: https://github.com/apache/superset/pull/25557
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Improves the Waterfall chart [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #25557:
URL: https://github.com/apache/superset/pull/25557#discussion_r1377599830
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -17,24 +17,26 @@
* under the License.
*/
import React from 'react';
-import { ensureIsArray, t } from '@superset-ui/core';
+import { t } from '@superset-ui/core';
import {
ControlPanelConfig,
+ ControlSubSectionHeader,
+ D3_TIME_FORMAT_DOCS,
+ DEFAULT_TIME_FORMAT,
formatSelectOptions,
- getStandardizedControls,
- sections,
+ sharedControls,
} from '@superset-ui/chart-controls';
import { showValueControl } from '../controls';
const config: ControlPanelConfig = {
controlPanelSections: [
- sections.legacyTimeseriesTime,
{
label: t('Query'),
expanded: true,
controlSetRows: [
- ['series'],
- ['columns'],
+ ['x_axis'],
+ ['time_grain_sqla'],
+ ['groupby'],
Review Comment:
Don't we need a migration so that the existing charts work with the new controls?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Improves the Waterfall chart [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #25557:
URL: https://github.com/apache/superset/pull/25557#issuecomment-1787546919
@michael-s-molina Ephemeral environment spinning up at http://34.219.21.113:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Improves the Waterfall chart [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #25557:
URL: https://github.com/apache/superset/pull/25557#discussion_r1377844469
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -17,24 +17,26 @@
* under the License.
*/
import React from 'react';
-import { ensureIsArray, t } from '@superset-ui/core';
+import { t } from '@superset-ui/core';
import {
ControlPanelConfig,
+ ControlSubSectionHeader,
+ D3_TIME_FORMAT_DOCS,
+ DEFAULT_TIME_FORMAT,
formatSelectOptions,
- getStandardizedControls,
- sections,
+ sharedControls,
} from '@superset-ui/chart-controls';
import { showValueControl } from '../controls';
const config: ControlPanelConfig = {
controlPanelSections: [
- sections.legacyTimeseriesTime,
{
label: t('Query'),
expanded: true,
controlSetRows: [
- ['series'],
- ['columns'],
+ ['x_axis'],
+ ['time_grain_sqla'],
+ ['groupby'],
Review Comment:
Ah thanks for clarification 👍
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -17,24 +17,26 @@
* under the License.
*/
import React from 'react';
-import { ensureIsArray, t } from '@superset-ui/core';
+import { t } from '@superset-ui/core';
import {
ControlPanelConfig,
+ ControlSubSectionHeader,
+ D3_TIME_FORMAT_DOCS,
+ DEFAULT_TIME_FORMAT,
formatSelectOptions,
- getStandardizedControls,
- sections,
+ sharedControls,
} from '@superset-ui/chart-controls';
import { showValueControl } from '../controls';
const config: ControlPanelConfig = {
controlPanelSections: [
- sections.legacyTimeseriesTime,
{
label: t('Query'),
expanded: true,
controlSetRows: [
- ['series'],
- ['columns'],
+ ['x_axis'],
+ ['time_grain_sqla'],
+ ['groupby'],
Review Comment:
Ah thanks for clarification 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Improves the Waterfall chart [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #25557:
URL: https://github.com/apache/superset/pull/25557#issuecomment-1787542992
> Code and basic testing LGTM.
Thank you for the review @kgabryje!
> Should we have more comprehensive qa session before publishing the viz?
That would be great! I'll spin up a test environment and leave the PR open for some days. @jinghua-qa @sadpandajoe @rusackas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Improves the Waterfall chart [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #25557:
URL: https://github.com/apache/superset/pull/25557#issuecomment-1787867522
> I agree. It will look much better. Let's address this in a follow-up to test all scenarios where this component is being used.
Follow-up: https://github.com/apache/superset/pull/25812
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Improves the Waterfall chart [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #25557:
URL: https://github.com/apache/superset/pull/25557#issuecomment-1787543234
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Improves the Waterfall chart [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #25557:
URL: https://github.com/apache/superset/pull/25557#issuecomment-1787244279
I know it's an existing component so we can address it in a separate PR, but it looks very weird - like a popover inside of a popover. I think it'd look much better if we styled it a bit by removing the border from the color picker component and giving it the same background color as the antd popover
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Improves the Waterfall chart [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #25557:
URL: https://github.com/apache/superset/pull/25557#discussion_r1377832264
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx:
##########
@@ -17,24 +17,26 @@
* under the License.
*/
import React from 'react';
-import { ensureIsArray, t } from '@superset-ui/core';
+import { t } from '@superset-ui/core';
import {
ControlPanelConfig,
+ ControlSubSectionHeader,
+ D3_TIME_FORMAT_DOCS,
+ DEFAULT_TIME_FORMAT,
formatSelectOptions,
- getStandardizedControls,
- sections,
+ sharedControls,
} from '@superset-ui/chart-controls';
import { showValueControl } from '../controls';
const config: ControlPanelConfig = {
controlPanelSections: [
- sections.legacyTimeseriesTime,
{
label: t('Query'),
expanded: true,
controlSetRows: [
- ['series'],
- ['columns'],
+ ['x_axis'],
+ ['time_grain_sqla'],
+ ['groupby'],
Review Comment:
This chart is not officially released yet. The previous PR was an alpha version with many features missing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Improves the Waterfall chart [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #25557:
URL: https://github.com/apache/superset/pull/25557#issuecomment-1787523791
> I know it's an existing component so we can address it in a separate PR, but it looks very weird - like a popover inside of a popover. I think it'd look much better if we styled it a bit by removing the border from the color picker component and giving it the same background color as the antd popover
I agree. It will look much better. Let's address this is a follow-up to test all scenarios where this component is being used.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org