fix(FilterBox): treat empty defaultValues as null #13109
Conversation
| [FILTER_CONFIG_ATTRIBUTES.DEFAULT_VALUE]: defaultValue, | ||
| [FILTER_CONFIG_ATTRIBUTES.CLEARABLE]: isClearable, | ||
| [FILTER_CONFIG_ATTRIBUTES.SEARCH_ALL_OPTIONS]: searchAllOptions, | ||
| } = filterConfig; |
There was a problem hiding this comment.
I also did some light refactoring to make the code easier to read.
| // empty values are ignored | ||
| if (!selectedValues[key]) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This will fix this bug:
Before
After
| '28 days ago', | ||
| '90 days ago', | ||
| '1 year ago', | ||
| ]; |
There was a problem hiding this comment.
Bycatch: this constant is not used anywhere.
ktmud
force-pushed
the
filterbox-empty-as-null
branch
from
54908a0
to
390eb31
Compare
Codecov Report
@@ Coverage Diff @@
## master #13109 +/- ##
==========================================
+ Coverage 53.06% 53.42% +0.36%
==========================================
Files 489 494 +5
Lines 17314 17373 +59
Branches 4482 4503 +21
==========================================
+ Hits 9187 9281 +94
+ Misses 8127 8092 -35
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Thanks for the fix and house keeping! |
|
@junlincc This is because the default time range filter ("Last week") was applied to your FilterBox chart. This is a legacy behavior I'm also not a fan of, but it is unrelated to this bug. At Airbnb, we've discussed whether to allow setting a default Time Range filter per datasource as simply removing this default filter may have performance impact on some datasets, but we never had the time to follow it though. Could be a nice roadmap item, though. |
|
hmm. i removed time filter but still not able to see the default value in country. @ktmud |
|
In Explore view, default values are not displayed in the rendered chart, but they will be applied in the dashboard. This is a known issue and also a separate bug. Feel free to create a new issue to track it, or I'll do some investigation when I have time. |
ktmud
force-pushed
the
filterbox-empty-as-null
branch
from
390eb31
to
5b60c8b
Compare
|
@ktmud @junlincc I was also quite surprised by the default values not showing up on Explore (I haven't used them that much). I would vote for not putting effort into implementing default values in Explore view unless it's trivial, as the native select filter is almost ready for prime time and IMO provides a much cleaner UX than this component. |
I agree with @villebro . Grace and Zuzana, native filter is reaching feature parity in about 1 week. at that point, we will have our main focus on the new feature set and may not actively maintain FilterBox anymore(i don't think we will ever deprecate it though). Now is a good time to start the migration script as we discussed before. |
* fix(FilterBox): treat empty defaultValues as null * Add a unit test and move constant around (cherry picked from commit 613945a)
* fix(FilterBox): treat empty defaultValues as null * Add a unit test and move constant around (cherry picked from commit 613945a)
mistercrunch
added
🏷️ bot
SUMMARY
Fixes #13099
FilterBoxwill send empty string as filter values when a column'sdefaultValueis empty string, causing no-results for filtered charts.The default value for filter column's
defaultValueisnull, but if users have edited it, it will be updated to an empty string.This empty string is then propagated to
extra_filtersof the affected charts. This PR will treat empty strings asnullwhen constructing filter values for the dashboard.This is the preferred fix because there may be existing
FilterBoxcharts already havedefaultValue: ""saved in their metadata.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
See #13099 how to reproduce the bug.
TEST PLAN
ADDITIONAL INFORMATION