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

feat: Improves the Waterfall chart #25557

Merged
merged 13 commits into from
Nov 3, 2023
Merged

feat: Improves the Waterfall chart #25557

merged 13 commits into from
Nov 3, 2023

Conversation

michael-s-molina
Copy link
Member

@michael-s-molina michael-s-molina commented Oct 6, 2023 •
edited
Loading

SUMMARY

This PR improves the Waterfall chart developed in #17906 with the following changes:

1 - Improves the chart description and provide example thumbnails

Screenshot 2023-10-19 at 14 51 47


2 - Fixes a bug with negative values where the bars are displayed above the 0 line

Screenshot 2023-10-18 at 16 03 20


3 - Augments the tooltip with intermediate totals

Screenshot 2023-10-18 at 16 04 26


4 - Adds support for generic chart axes

Screenshot 2023-10-18 at 16 05 08


5 - Adds support for series color selection

Screenshot 2023-10-18 at 16 05 45


6 - Adds support for currency formatting

Screenshot 2023-10-18 at 16 06 16


7 - Adds support for time formatting

Screenshot 2023-10-18 at 16 06 46


8 - Adds support for x-axis with mixed data types when using breakdowns

Screenshot 2023-10-18 at 16 08 36


Hat tip to @rusackas for testing the plugin and documenting the issues here and @stephenLYZ for creating the first version of the plugin.

TESTING INSTRUCTIONS

1 - Create a Waterfall chart
2 - Test supported controls
3 - Test supported dashboard interactions

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 michael-s-molina requested review from stephenLYZ and removed request for stephenLYZ October 6, 2023 14:32
@michael-s-molina michael-s-molina changed the title feat: Improves the Waterfall chart feat: Finishes the Waterfall chart Oct 11, 2023
@michael-s-molina michael-s-molina changed the title feat: Finishes the Waterfall chart feat: Improves the Waterfall chart Oct 11, 2023
@pull-request-size pull-request-size bot added size/L and removed size/M labels Oct 11, 2023
@michael-s-molina michael-s-molina marked this pull request as ready for review October 19, 2023 17:53
kgabryje
kgabryje reviewed Oct 31, 2023
View reviewed changes
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/controlPanel.tsx
['columns'],
['x_axis'],
['time_grain_sqla'],
['groupby'],
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need a migration so that the existing charts work with the new controls?

Copy link
Member Author

Choose a reason for hiding this comment

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

This chart is not officially released yet. The previous PR was an alpha version with many features missing.

Copy link
Member

Choose a reason for hiding this comment

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

Ah thanks for clarification 👍

@kgabryje
Copy link
Member

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

@michael-s-molina
Copy link
Member Author

michael-s-molina commented Oct 31, 2023 •
edited
Loading

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 in a follow-up to test all scenarios where this component is being used.

kgabryje
kgabryje approved these changes Oct 31, 2023
View reviewed changes
Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

Code and basic testing LGTM. Should we have more comprehensive qa session before publishing the viz?

@michael-s-molina
Copy link
Member Author

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

@michael-s-molina
Copy link
Member Author

/testenv up

@github-actions GitHub Actions
Copy link
Contributor

@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.

@michael-s-molina
Copy link
Member Author

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: #25812

@michael-s-molina michael-s-molina merged commit d619078 into apache:master Nov 3, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 3, 2023

Ephemeral environment shutdown and build artifacts deleted.

@michael-s-molina michael-s-molina mentioned this pull request Nov 29, 2023
Merged
9 tasks
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@michael-s-molina
feat: Improves the Waterfall chart (apache#25557)
03b4ac0
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
@michael-s-molina @sfirke
feat: Improves the Waterfall chart (apache#25557)
c240ab1
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
@michael-s-molina
feat: Improves the Waterfall chart (apache#25557)
915aaeb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgabryje kgabryje kgabryje approved these changes

@geido geido Awaiting requested review from geido geido is a code owner

@rusackas rusackas Awaiting requested review from rusackas

@justinpark justinpark Awaiting requested review from justinpark

@john-bodley john-bodley Awaiting requested review from john-bodley

@villebro villebro Awaiting requested review from villebro

@jinghua-qa jinghua-qa Awaiting requested review from jinghua-qa

@stephenLYZ stephenLYZ Awaiting requested review from stephenLYZ

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XL 🚢 3.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@michael-s-molina @kgabryje @mistercrunch