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: conditional coloring for big number chart #23064

Merged
merged 3 commits into from
May 1, 2023

Conversation

gbusch
Copy link
Contributor

@gbusch gbusch commented Feb 12, 2023

SUMMARY

As suggested here: #21820
allow for conditional formatting (coloring) of Big Number charts.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screenshot 2023-02-12 at 15 58 36

TESTING INSTRUCTIONS

  • select a "Big Number" chart
  • add "conditional formatting" rules in the "Customize" tab

ADDITIONAL INFORMATION

re-uses existing "ConditionalFormattingControl" from chart table and pivot table

@codecov
Copy link

codecov bot commented Feb 12, 2023

Codecov Report

Merging #23064 (999198f) into master (17fbb2d) will decrease coverage by 1.19%.
The diff coverage is 48.87%.

❗ Current head 999198f differs from pull request most recent head c073c0b. Consider uploading reports for the commit c073c0b to get more accurate results

@@            Coverage Diff             @@
##           master   #23064      +/-   ##
==========================================
- Coverage   67.47%   66.28%   -1.19%     
==========================================
  Files        1880     1922      +42     
  Lines       72283    74068    +1785     
  Branches     7881     8115     +234     
==========================================
+ Hits        48772    49098     +326     
- Misses      21486    22895    +1409     
- Partials     2025     2075      +50     
Flag Coverage Δ
hive ?
javascript 54.11% <48.87%> (+0.21%) ⬆️
presto ?
unit ?

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

Impacted Files Coverage Δ
...et-ui-chart-controls/src/operators/sortOperator.ts 100.00% <ø> (ø)
...t-ui-chart-controls/src/shared-controls/mixins.tsx 16.66% <ø> (ø)
...d/packages/superset-ui-chart-controls/src/types.ts 100.00% <ø> (ø)
.../packages/superset-ui-core/src/chart/types/Base.ts 100.00% <ø> (ø)
...-core/src/hooks/useChangeEffect/useChangeEffect.ts 100.00% <ø> (ø)
...hooks/useComponentDidMount/useComponentDidMount.ts 100.00% <ø> (ø)
...oks/useComponentDidUpdate/useComponentDidUpdate.ts 100.00% <ø> (ø)
...src/hooks/useElementOnScreen/useElementOnScreen.ts 100.00% <ø> (ø)
...erset-ui-core/src/hooks/usePrevious/usePrevious.ts 100.00% <ø> (ø)
...re/src/hooks/useTruncation/useCSSTextTruncation.ts 100.00% <ø> (ø)
... and 128 more

... and 478 files with indirect coverage changes

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

@rusackas
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

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

@rusackas
Copy link
Member

Code looks good, and it seems to work well on the ephemeral environment. The only question I have is whether it's possible/reasonable to make the colors full opacity rather than the sort of pastel colors that they are now. I'm a little worried about contrast/legibility in particular.

@justin-tomlinson
Copy link

Very useful improvement. It would be great if this was available across the Big Number and Big Number with trend line. Some additional improvements to functionality could be:

  • Make the colour scheme be more configurable by using hex codes or RGB values
  • Making the colour selection gradient optional. sometimes you want it to be a set colour when it meets the rule rather than be some gradient of the colour. I.e If measure >0.5 then Red, not a gradient of red.

This probably applies to the conditional formatting on the table chart too. -

@rusackas
Copy link
Member

@gbusch curious about your input on any of the above comments... would love to see this PR get through! :)

@gbusch
Copy link
Contributor Author

gbusch commented Apr 15, 2023

Sorry, was busy and then almost forgot about it...

Colors are defined in this file:
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx

I renamed the colors to make it clearer that they refer to the theme colors and added possibility for darker colors (from theme). Does this make sense?

@gbusch
Copy link
Contributor Author

gbusch commented Apr 24, 2023

@rusackas What do you think?
Pipeline is red... Don't understand why, as the changes are rather small.

@kgabryje
Copy link
Member

Pipeline is red... Don't understand why, as the changes are rather small.

The E2E tests are sometimes flaky... I restarted the CI, hopefully it passes this time

@gbusch gbusch requested a review from rusackas April 27, 2023 10:14
@rusackas rusackas merged commit 61d8a0b into apache:master May 1, 2023
@gbusch gbusch deleted the gbusch/feature/coloring_big_number branch May 1, 2023 15:48
@mtrentz
Copy link

mtrentz commented Sep 13, 2023

Big Numbers doesn't get color formatted when its value is zero. Anyone else experience this?

If I set a rule for "error when < 5" and the value is 0, nothing happens.

If the value is 0.0001 then it colors.

@rusackas
Copy link
Member

rusackas commented Feb 1, 2024

Big Numbers doesn't get color formatted when its value is zero. Anyone else experience this?

If I set a rule for "error when < 5" and the value is 0, nothing happens.

If the value is 0.0001 then it colors.

Would you be willing to open a PR or an Issue about this? We haven't heard much outcry, and would love any help you can provide to set it on a course toward resolution.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024
@mtrentz
Copy link

mtrentz commented Apr 15, 2024

@rusackas opened this one #28038

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/L 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants