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

feat(plugins): Make comparison values on BigNumberPeriodOverPeriod toggleable #28605

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mkramer5454
Copy link
Contributor

SUMMARY

There are various scenarios where it is preferred to only show a subset of the three comparison values: previous period value, period difference, and period percent change. This change allows for hiding unwanted values.

(This is my first contribution on the frontend side so please let me know if I missed something obvious!)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

optional_comparison_diplays.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags: CHART_PLUGINS_EXPERIMENTAL
  • 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

@dosubot dosubot bot added the viz:charts:bignumber Related to BigNumber charts label May 20, 2024
@eschutho
Copy link
Member

/testenv up

@Antonio-RiveroMartnez
Copy link
Member

/testenv up FEATURE_CHART_PLUGINS_EXPERIMENTAL=true

Copy link
Contributor

@Antonio-RiveroMartnez Ephemeral environment spinning up at http://35.93.65.110:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@fontclos
Copy link

@eschutho any chance this gets reviewed and merged? We'd very much love the feature!

@fontclos
Copy link

/testenv up FEATURE_CHART_PLUGINS_EXPERIMENTAL=true

Copy link
Contributor

@fontclos Ephemeral environment creation is currently limited to committers.

@rusackas
Copy link
Member

/testenv up FEATURE_CHART_PLUGINS_EXPERIMENTAL=true

Copy link
Contributor

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

@eschutho
Copy link
Member

eschutho commented Dec 6, 2024

Thanks for the contribution @mkramer5454! This looks good to me. Can you write a few tests here: superset-frontend/packages/superset-ui-core/test/time-comparison/getComparisonInfo.test.ts

@eschutho
Copy link
Member

eschutho commented Dec 6, 2024

And maybe one small UI improvement, when "Add color for positive/negative" is selected, there's an extra dialog that shows up. I'd suggest to keep the two fields together and minimize any jumpiness, we just move the "Add color" field to the bottom below the new checkboxes. Also cc @yousoph and @kasiazjc on any other design suggestions.
Screenshot 2024-12-05 at 4 52 58 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants