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(world-map): support color by metric or country column #19881

Merged
merged 7 commits into from
Jun 3, 2022

Conversation

stephenLYZ
Copy link
Member

@stephenLYZ stephenLYZ commented Apr 28, 2022

SUMMARY

This PR allows world map to support country color in two ways:

  • color by metric (same as before): use gradient color palettes
  • color by country column (new): use categorical color palettes

Here we add a radio control to achieve it.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before

image

after

2022-04-28.8.43.23.mov

TESTING INSTRUCTIONS

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

@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #19881 (4557081) into master (d7e3ac3) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #19881      +/-   ##
==========================================
- Coverage   66.46%   66.44%   -0.03%     
==========================================
  Files        1721     1721              
  Lines       64517    64542      +25     
  Branches     6807     6811       +4     
==========================================
+ Hits        42879    42882       +3     
- Misses      19906    19927      +21     
- Partials     1732     1733       +1     
Flag Coverage Δ
javascript 51.31% <0.00%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
.../shared-controls/components/RadioButtonControl.tsx 0.00% <ø> (ø)
...gins/legacy-plugin-chart-world-map/src/WorldMap.js 0.00% <0.00%> (ø)
.../legacy-plugin-chart-world-map/src/controlPanel.ts 33.33% <0.00%> (-66.67%) ⬇️
...egacy-plugin-chart-world-map/src/transformProps.js 0.00% <0.00%> (ø)
...end/src/dashboard/components/SliceHeader/index.tsx 86.27% <0.00%> (-0.96%) ⬇️
...et-frontend/src/components/EditableTitle/index.tsx 70.00% <0.00%> (ø)
...ontend/src/views/CRUD/data/dataset/DatasetList.tsx 55.31% <0.00%> (ø)
.../src/explore/components/ControlPanelsContainer.tsx 79.80% <0.00%> (ø)
...hart-pivot-table/src/react-pivottable/utilities.js 0.00% <0.00%> (ø)
...ts/nativeFilters/FilterBar/ActionButtons/index.tsx 85.71% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7e3ac3...4557081. Read the comment docs.

@rusackas
Copy link
Member

Two random thoughts... and these can be separate tickets/PRs if we want to do either of them:

  1. This is the weird one: If you select to color the map by country, and deselect select "show bubbles," then you don't really need the Metric input (which is currently a required input). I thought for a second about making that input optional in that case... but in this configuration, the map is essentially useless, and doesn't tell you anything... so... whatever - fine as is. ¯\_(ツ)_/¯

2a) You only actually need four colors to display a map. As a default, it might be nice to take advantage of that, if your mapping library happens to support the four color theorem as a feature.

2b) If we do take advantage of the four color theorem, it may get ugly with color consistency... so... I don't quite know what to do there.

  1. Also, with the color consistency thing... since a map might gobble up all the colors of a palette instantly, is there a good way to de-prioritize this chart (or others like it) it in the color assignment process?

processedData = filteredData.map(d => ({
...d,
radius: radiusScale(Math.sqrt(d.m2)),
fillColor: colorScale(d.name, sliceId),
Copy link
Member

Choose a reason for hiding this comment

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

We could also base this on the metric, rather than the name, so that the country with the highest value gets the first color in the palette.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. But actually we may need to use country name as the color label instead of metric value when we use categorical color, and the metric values here are not in order. If we want to make the country with the highest value get the first color in the palette, we can sort the filteredData first.

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.

Left a bunch of thoughts/comments, but I'm ready to approve after those get considered :)

stephenLYZ and others added 4 commits April 29, 2022 17:51
…ed-controls/components/RadioButtonControl.tsx

Co-authored-by: Evan Rusackas <evan@preset.io>
…ntrolPanel.ts

Co-authored-by: Evan Rusackas <evan@preset.io>
…ntrolPanel.ts

Co-authored-by: Evan Rusackas <evan@preset.io>
@stephenLYZ stephenLYZ requested a review from zhaoyongjie May 10, 2022 15:37
@stephenLYZ stephenLYZ merged commit 766f737 into apache:master Jun 3, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
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 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants