fix(native-filters): improve loading styles for filter component

[villebro]

SUMMARY

Currently the native filter components render irregularly when waiting for data to be returned from the database, causing unnecessary zigzagging on the filter tab. In addition, the default value selector in the filter config modal doesn't have a spinner to indicate that data is loading.

AFTER

Now the minimum size of the filter item is at minimum the same height as the AntD select component, leading to a more peaceful loading experience. Also note smaller spinner:

When data is being fetched from the backend in the filter config modal, the spinner is activated:

Before

Previously the filter item would contract when the filter component had finished retrieving data but had not yet fully rendered the component:

The default value selector didn't show that data was being awaited from the backend, but rather displayed "No results":

TEST PLAN

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

[villebro]

The min height needs to be 32px + the bottom padding, which in gridUnits is 8 + 2.5 = 10.5.

[rusackas]

I'm hoping/trying to avoid fractional gridUnits if at all possible. I know there are a few kicking around in the codebase, but (a) we should just learn to stick to the grid for the sake of design's sanity, and (b) ththings can get weird if gridUnit is ever moved to, say, 5px, and we find ourselves in fractional-pixel land.

[villebro]

Ha - I looked at the designs, and turns out they were more like 12 than 10 - updating accordingly.

[rusackas]

Same with the line above btw. Maybe 10px is fine?

[villebro]

This needed to be moved inside the FilterItem, otherwise the title disappears when loading.

[codecov]

Codecov Report

Merging #13794 (9c90885) into master (18ff484) will increase coverage by 0.33%.
The diff coverage is 92.59%.

@@            Coverage Diff             @@
##           master   #13794      +/-   ##
==========================================
+ Coverage   77.22%   77.56%   +0.33%     
==========================================
  Files         935      935              
  Lines       47266    47276      +10     
  Branches     5893     5905      +12     
==========================================
+ Hits        36502    36668     +166     
+ Misses      10616    10464     -152     
+ Partials      148      144       -4     

Flag	Coverage Δ
cypress	56.06% <100.00%> (-3.95%)	⬇️
javascript	63.75% <62.96%> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
superset-frontend/src/components/Loading/index.tsx	100.00% <ø> (ø)
...tersConfigModal/FiltersConfigForm/DefaultValue.tsx	93.10% <90.00%> (+2.62%)	⬆️
...veFilters/FilterBar/FilterControls/FilterValue.tsx	85.18% <100.00%> (-0.27%)	⬇️
...ters/FiltersConfigModal/FiltersConfigForm/state.ts	89.47% <100.00%> (+0.58%)	⬆️
...set-frontend/src/dashboard/util/getDropPosition.js	90.90% <0.00%> (-1.52%)	⬇️
...erset-frontend/src/SqlLab/components/ResultSet.tsx	68.64% <0.00%> (-0.30%)	⬇️
...rontend/src/SqlLab/components/AceEditorWrapper.tsx	61.25% <0.00%> (+1.25%)	⬆️
...rontend/src/visualizations/FilterBox/FilterBox.jsx	73.58% <0.00%> (+1.25%)	⬆️
...erset-frontend/src/SqlLab/components/SqlEditor.jsx	56.01% <0.00%> (+1.38%)	⬆️
...ntend/src/explore/components/ExploreChartPanel.jsx	82.60% <0.00%> (+1.44%)	⬆️
... and 18 more

---

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 18ff484...9c90885. Read the comment docs.

[junlincc]

LGTM! I will need to test it again in master, if everything looks good - after +ing compatibility with sync query, let's turn native filter on by default for Superset users :D @villebro this is awesome!

[rusackas]

It might be worth trying

Suggested change

	<Loading />
	<Loading position="inline" />

to see if it's a little smaller, and sits well. It might be left aligned, but if that looks strange, then StyledLoadingBox could also have text-align: center;

[rusackas]

Actually, now that I look at this again, if you do the <Loading position="inline" /> bit, then you no longer need the position: relative; or height: ${({ theme }) => theme.gridUnit * 8}px; styles. This feels a bit cleaner to me.

Of course... it may be worth pointing out that if you don't mind the Loading icon being left-aligned, then you could skip that text-align: center; style.... which means you don't need the StyledLoadingBox div at all... which means you don't need the LoadingBox component at all.

And now, to completely belabor this...
If you DO want the loading centered, but think you might want that to be highly reusable, and don't want to bother with adding the LoadingBox component, then you can edit src/component/Loading/index.tsx and add the functionality there:

1. Add an inline-centered class following the inline class:

&.inline {
    margin: 0px;
    width: 30px;
  }
  &.inline-centered {
    margin: 0 auto;
    width: 30px;
    display: block;
  }

2. Update PositionOption to include inline-centered
3. Simply use <Loading position="inline-centered" /> in FilterValue.tsx instead of <LoadingBox />

[rusackas]

I added a bunch of comments on how to remove some of this new code, and instead extend the Loading component with a centered version, in case you want to make this all a bit smaller. If you do, please update the Loading Storybook entry too.

That said, what you have works quite well, and I've tested it locally to confirm. Approving.

[villebro]

Thanks @rusackas! I'll address all the low hanging fruit from your comments today and get this merged to unblock pending follow-up PRs (we can further improve the aesthetics in future PRs)