[Feature Anywhere] Fix bug of tooltip showing 'undefined' in certain cases #4281
Conversation
Signed-off-by: Tyler Ohlsen <[email protected]>
ohltyler
requested review from
ananzh,
kavilla,
seanneumann,
AMoo-Miki,
ashwin-pc,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
kristenTian,
zhongnansu and
manasvinibs
as code owners
ohltyler
changed the title
ohltyler
requested review from
lezzago
and removed request for
seanneumann,
AMoo-Miki,
kavilla,
zhongnansu,
abbyhu2000,
zengyan-amazon,
ananzh,
kristenTian and
manasvinibs
Signed-off-by: Tyler Ohlsen <[email protected]>
|
Tests pass for these changes. Existing test failures are fixed in #4269 - can rebase there to ensure all are passing after that one is merged. |
| // below case should not happen since only VisLayers with a populated | ||
| // set of events should be passed from the plugins. but, if it does | ||
| // happen, we can handle it more gracefully instead of throwing an error | ||
| it('vis layer with empty events adds nothing to datatable', function () { |
There was a problem hiding this comment.
Why are we removing this test? Is it because it would not be empty and have placeholder 0 values? If so, we should modify the test for that.
There was a problem hiding this comment.
The test is invalid now, since it was testing that nothing would be added to the datatable. But now we add 0s. The rest of the tests handle such scenarios
There was a problem hiding this comment.
Additionally the test was hacky to begin with. The scenario should never happen and other tests to ensure the logic should never reach this state are already tested.
Codecov Report
@@ Coverage Diff @@
## feature/feature-anywhere #4281 +/- ##
============================================================
+ Coverage 66.29% 66.35% +0.05%
============================================================
Files 3271 3271
Lines 62943 62947 +4
Branches 9741 9741
============================================================
+ Hits 41731 41766 +35
+ Misses 18865 18838 -27
+ Partials 2347 2343 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Signed-off-by: Tyler Ohlsen <[email protected]>
Description
There is a bug where the event tooltips will show 'undefined' when there are multiple source resource types, but not all are populated for a particular time bucket. This fixes that by pre-populated 0 values for all resource type columns, such that when any are non-zero, the rest will show as 0.
Note there is some optimizations and potential vega upgrades that can solve this, but the current solution would require modifying the spec at the vega-level to implement the tooltip via
signals. To keep things simplified, we go with this approach to keep things at the abstracted vega-lite level (example of using signals to validate datapoints: see here)Screenshot
Before:
After:
Check List
yarn test:jestyarn test:jest_integrationyarn test:ftr