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(superset-frontend): color formats big numbers when value is 0 #28038

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mtrentz
Copy link

@mtrentz mtrentz commented Apr 15, 2024

SUMMARY

Previously when the value of a BigNumber was zero (falsey) the expression to evaluate colors always got evaluated to false. Meaning that if my condition was number <= 50 and my number was 0, it wouldn't color it. It would work if it was 0.1.

Related to this issue: #21820
Most recent discussion around this PR #23064

TESTING INSTRUCTIONS

  • Tested if colors when BigNumber is zero
  • Tested if still works for negative, floats, and scientific notation numbers like 1e-100

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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@rusackas rusackas requested a review from kgabryje April 15, 2024 17:30
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

Looks straightforward to me... though a test would help against regressions, if you don't mind adding one.

@rusackas
Copy link
Member

Thanks for opening this!

@john-bodley
Copy link
Member

@mtrentz thanks for the fix. Would you mind adding unit tests to prevent future regressions?

@mtrentz
Copy link
Author

mtrentz commented Apr 24, 2024

Hey @john-bodley and @rusackas

I would be happy to contribute with tests but in this case I don't know how.

The logic I've changed is inside the rendering logic and I don't know how to mock render these e-charts. I also could not find any other similar tests that I can use to base myself of when testing this part of the code.

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.

3 participants