-
Notifications
You must be signed in to change notification settings - Fork 272
fix(plugin-chart-word-cloud): ensure top results are always displayed #841
fix(plugin-chart-word-cloud): ensure top results are always displayed #841
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/j6yybjmdb |
Codecov Report
@@ Coverage Diff @@
## master #841 +/- ##
==========================================
- Coverage 26.73% 26.67% -0.06%
==========================================
Files 405 405
Lines 8248 8266 +18
Branches 1126 1128 +2
==========================================
Hits 2205 2205
- Misses 5914 5932 +18
Partials 129 129
Continue to review full report at Codecov.
|
Thanks for the quick fix @agatapst! this is awesome! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @agatapst , looks really good! While this will have a performance impact, I think it's worth it given how misleading the results are without this fix.
A few suggestions:
- I've noticed that Word Cloud is especially poor at rendering long "words". For instance, The value "Latin America and Caribbean Islands" will tend to not render at all if it's the only value unless the chart is really big. So having a story in the story book with a few really long words would be great.
- The logic for ensuring that at minimum the first 10 % values are present is slightly difficult to follow. Do you think we could break out the relevant utility function(s) and add a few unit tests to make sure it works with e.g. 1, 5, 20 and 100 values, just to make sure the rounding works correctly?
@villebro Thanks for your suggestions, I will think that through and come back with some ideas soon. 🙂 |
@agatapst this would be great to get merged, but needs a rebase. |
While resizing chart sometimes top results were filtered out because their sizes were too big. This solution makes sure that top 10% of results will always be displayed by gradually scaling down the chart if needed. fix apache/superset#11784
380f649
to
1bfeec9
Compare
@villebro sorry for the delay! it's rebased now |
Thanks @agatapst! |
@villebro all checks have passed, if PR is ok without any changes, it's ready to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a reasonable solution.
While resizing chart sometimes top results were filtered out because their sizes were too big. This
solution makes sure that top 10% of results will always be displayed by gradually scaling down the
chart if needed.
fix apache/superset#11784
🐛 Bug Fix
Before:
After:
Issues associated with this bug were reported in D3 library and not completely solved yet.
- most important words (highest counts) missing if they don't fit
- missing words in final layout
The cost of my workaround solution may be slower pace of displaying the chart while resizing.
cc @villebro