feat(listview): skeleton loading states for table and card collections

[nytai]

SUMMARY

• render 25 row skeleton on initial load
• toggle skeleton state on existing components when fetching new data.
• remove row count text when loading.
• use ImageLoader component to:
  1. render fallback image while fetching image.
  2. render fallback image if actual image returns an error.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

• WIP

ADDITIONAL INFORMATION

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

[nytai]

this is an internal dependency for fetchMock. Installing this was necessary in order to mock blob responses

[mistercrunch]

[optional] this would be an excellent comment to squeeze into the package.json if json supported comments. There are some hacky ways to add comments. https://coding.napolux.com/how-to-add-comments-to-package-json/

[nytai]

I'm not in favor of hacking in json comments. This is installed under devDependencies and the git blame should be enough to trace the change back to this PR.

[rusackas]

package.json5 -- one can dream, right? ;)

[mistercrunch]

NIT: use a class or a constant

[mistercrunch]

NIT: class or constant. double diamonds {{ ... }} are generally suboptimal as they get re-created on each render and also trigger re-render in PureComponents (are they still popular?)

[nytai]

You are correct, they were generally not favorable (though I'm not so sure anymore with the rise in popularity of functional components and React's move to async rendering). However, these components just end up rendering divs with some loading shimmer styles, they're about a simple/dumb as components get. It won't have a measurable impact on performance in this instance. I'll move them to emotion though for consistency with other components.

[rusackas]

You can now add ListViewCard.test.jsx (and/or other *.test.(j|t)sx files next to the component file itself (in the same folder), to make it easier to find/manage, and make it easier to import anything from the ListViewCard.stories.jsx file that may be useful in scaffolding tests.

See: #10634

[rusackas]

This seems fine, but often people just use the named import of useEffect - I'm not sure if we have a pattern we want to follow/establish

[rusackas]

5px? Should probably also be based on gridUnit... which would normally be 4... so this seems a bit odd.

[rusackas]

Might it be a little DRYer to have the conditional rendering loading && ( down here within ? Not sure if that would be easier or harder to interpret ¯\_(ツ)_/¯

[nytai]

I think I had something like that originally but found it quite hard to read. I think the slightly less DRYness is worth if for higher code readability. It's much easier to figure out "Am I dealing with the skeleton or the actual component" if the two are separate blocks of jsx.

[rusackas]

+px?
Also, again not sure if this big value should be a % or vw value rather than hard-coded

Last but not least though, I'm wondering if these css calls are somehow memoized to use the same class, or if it's generating a bunch of them here, meaning we should do something outside the function. Hopefully this is fine, but maybe worth checking for a performance hit.

[nytai]

I'm pretty sure they're statically extracted and replaced with classnames, setting this up required changes to babel which would indicate static code analysis/transformation of some sort.

Also, emotion is advertised as a performant css-in-js solution, so I'd expect them to have heavily optimized this use case.

[nytai]

here's some literature around this, there's a section where they mention that by default everything is extracted into static styles using css vars where needed.

https://medium.com/@tkh44/emotion-ad1c45c6d28b

[rusackas]

same px/gridUnit question. Sorry to beat a dead horse.

[rusackas]

Looking good in general. Some nits/questions listed, but the main thing is that I'd like to see some form of CSS units on numeric values. Now that there's a DOCTYPE, we can't rely on quirks mode to assure that these numbers fall back to px

[codecov-commenter]

Codecov Report

Merging #10606 into master will increase coverage by 0.02%.
The diff coverage is 89.74%.

@@            Coverage Diff             @@
##           master   #10606      +/-   ##
==========================================
+ Coverage   64.26%   64.29%   +0.02%     
==========================================
  Files         781      783       +2     
  Lines       36882    36935      +53     
  Branches     3499     3527      +28     
==========================================
+ Hits        23704    23746      +42     
- Misses      13069    13077       +8     
- Partials      109      112       +3     

Flag	Coverage Δ
#cypress	54.05% <ø> (-0.34%)	⬇️
#javascript	60.88% <89.74%> (+0.13%)	⬆️
#python	59.78% <ø> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
...c/components/ListViewCard/ListViewCard.stories.tsx	0.00% <0.00%> (ø)
...rontend/src/components/ListView/CardCollection.tsx	78.57% <80.00%> (+7.14%)	⬆️
...ontend/src/components/ListViewCard/ImageLoader.tsx	83.33% <83.33%> (ø)
superset-frontend/src/common/components/index.ts	100.00% <100.00%> (ø)
...rset-frontend/src/components/ListView/ListView.tsx	95.40% <100.00%> (+0.10%)	⬆️
...ontend/src/components/ListView/TableCollection.tsx	100.00% <100.00%> (ø)
...set-frontend/src/components/ListViewCard/index.tsx	93.47% <100.00%> (+7.36%)	⬆️
...perset-frontend/src/views/CRUD/chart/ChartList.tsx	76.19% <100.00%> (ø)
...rontend/src/views/CRUD/dashboard/DashboardList.tsx	74.00% <100.00%> (+0.17%)	⬆️
...et-frontend/src/SqlLab/reducers/getInitialState.js	33.33% <0.00%> (-16.67%)	⬇️
... and 14 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 878f06d...329c6fd. Read the comment docs.

[rusackas]

Impacts #8976