search

deephaven / web-client-ui

Deephaven Web Client UI
Apache License 2.0
28 stars 30 forks source link

fix: Fix missing scrim background on LoadingOverlay #2098

Closed vbabich closed 1 month ago

vbabich commented 1 month ago

Background class wasn't applied when LoadingOverlay has error or is loading

vbabich commented 1 month ago

I believe not having an overlay in this case was intentional - the scrim changes the colour unnecessarily of the panel if it hasn't loaded a widget yet. What ticket are we fixing here? This is for the error covering a dashboard?

This is for DH-16180 where I need a spinner with an overlay while the panel content is updating. I see that we do something like this in a couple of places:

<LoadingOverlay isLoading={showSpinner} isLoaded={!showSpinner} ... />

This displays the spinner without the overlay, then on the fade-out the black overlay is shown for a split second, so there's a bit of a flicker.

I guess the right usage should be just <LoadingOverlay isLoading={showSpinner} /> to show the spinner without the overlay, and <LoadingOverlay isLoaded isLoading={showSpinner} /> to show the spinner with the overlay. In that case, my change isn't needed.

For my use case, Don suggested adding a background-colored overlay immediately with no fade-in, and do a quick fade-out when loaded. I'm going to update the PR to parameterize background color, fade-in, and fade-out duration.

vbabich commented 1 month ago

Still, I think flickering the overlay when we change isLoading to false and isLoaded to true isn't right.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 46.63%. Comparing base (a30341a) to head (8c8de5e). Report is 16 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2098 +/- ## ========================================== + Coverage 46.59% 46.63% +0.03% ========================================== Files 679 682 +3 Lines 38598 38443 -155 Branches 9779 9580 -199 ========================================== - Hits 17986 17928 -58 + Misses 20560 20505 -55 + Partials 52 10 -42 ``` | [Flag](https://app.codecov.io/gh/deephaven/web-client-ui/pull/2098/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deephaven) | Coverage Δ | | |---|---|---| | [unit](https://app.codecov.io/gh/deephaven/web-client-ui/pull/2098/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deephaven) | `46.63% <100.00%> (+0.03%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=deephaven#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.