fix!: CSS based loading spinner by bmingles · Pull Request #1532 · deephaven/web-client-ui

bmingles · 2023-09-20T23:10:05Z

Swapped our LoadingSpinner implementation with a simpler html / CSS only spinner
Fixes a layout shift after font load caused by font-size: $iris-panel-icon-font-size in .message-icon of LoadingOverlay
Fixes loading spinner backtracking due to multiple instances of spinners

Regression Tests Performed

LoadingOverlay - I added a synthetic wait to PluginUtils right after the manifest loading. Loading overlay is now seamless. No layout shift or backtracking

Verified the following look as they did before

Community

Styleguide indeterminate and progress spinners
Styleguide context menu spinners
Console "Running" spinner
AppMainContainer disconnection spinner
Command history tooltip elapsed spinner
Advanced filter spinner
Column statistics spinner
IrisGridCopyHandler slight adjustment such that spinner + text are centered
PartitionSelectorSearch spinner
Commit button in PendingDataBottomBar
Custom column builder "applying" spinner
Table CSV exporter spinner

Enterprise

Enterprise login spinner

  await new Promise(resolve => {
      setTimeout(resolve, 2000);
  });

fixes #1531

BREAKING CHANGE: Inline LoadingSpinner instances will need to be decorated with className="loading-spinner-vertical-align" for vertical alignment to work as before

dsmmcken · 2023-09-21T13:35:44Z

+.loading-spinner {
+  --primary-color: var(--dh-accent-color, #4c7dee);
+  /* stylelint-disable-next-line alpha-value-notation */
+  --secondary-color: rgba(240, 240, 240, 0.5);


This needs to be based on a variable. A hardcoded value here would look terrible on a light theme for example.

Any thoughts on what it should be tied to?

in theory a gray-XXX value.

I'll tie it to a semantic color for now and refine it once I get into the color mapping

bmingles · 2023-09-21T15:31:35Z

+    gap: 20px;
    .message-icon {
      font-size: $iris-panel-icon-font-size;
+      height: $iris-panel-icon-font-size;


I set height + line height explicitly to avoid a layout shift after font load. The previous height was 96px based on 1.5 line-height from body. Converting to flexbox with a gap of 20px seems to match the layout as before but without the layout shift

mofojed · 2023-09-25T13:24:05Z

+ * @param animationName
+ * @param startTime


Params need descriptions. Also as it is now, you could make startTime default to 0 (since that's the only value you actually pass in).

mofojed · 2023-09-25T13:26:57Z

+  animations.forEach(a => {
+    // eslint-disable-next-line no-param-reassign
+    a.startTime = startTime;
+  });
+}


Not sure if I'm reading this incorrectly, but looking at the example on MDN: https://developer.mozilla.org/en-US/docs/Web/API/Animation/startTime#examples
They're setting the new animation to be the same as the existing start time; here you're resetting all of the animations to the same startTime (which is always 0, since that's the only value you pass in). Wouldn't that mean that an existing animation would jump back when a new one is added? Whereas we should just be syncing the new animation to any existing loading spinner that's already shown.

My observation when I dug into this is that the startTime is usually null by default (also was not what I expected from the docs). My understanding is that it's kind of a static value that determines the basis of when animations are calculated. Setting explicitly to zero later if the animation is already based on zero shouldn't impact the existing animation.

Interesting. Can't say I fully understand it, but it does look like they're in sync and doesn't reset the existing animations, so cool.

mofojed · 2023-09-25T13:29:31Z

      pendingElement = (
        <div className="loading">
-          <LoadingSpinner />
+          <LoadingSpinner className="mimic-fa-layers-vertical-align" />


Not crazy about needing to provide a long class name in many of these cases... should we have a verticalAlign prop or something that maps to a class name instead? Or any other way to do this?

I was thinking eventually we should use flex positioning for all of the cases and gradually get rid of the "hack" class. It's kind of a flag that this is styled in a deprecated way. That said, I'm not a big fan of it either, was just trying to not muck with too much of the existing layout. I'm fine to change this to a prop instead.

Or just a different class name; like we have loading-spinner-large, maybe loading-spinner-vertical-align

mofojed

Side note, I imagine we're going to move to Spectrum's ProgressCircle at some point for loading spinner: https://react-spectrum.adobe.com/react-spectrum/ProgressCircle.html
I wonder if that will cause this issue to come up again.

mofojed · 2023-09-27T13:52:26Z

A lot of the tests will need to be updated to look for this new loading spinner... we should have a consistent way of determining that in the tests. data-testid is probably the easiest; accessibility roles like the aria-live don't really apply to the loading spinner itself, just to the element behind it... whatever you think may be best.

bmingles · 2023-09-27T14:00:32Z

data-testid should be used the same way it was on the original spinner.

For aria roles, here's what Spectrum uses on their indeterminate spinner. I could see us adding those.

codecov · 2023-09-27T15:10:49Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (49723ec) 46.41% compared to head (77480a8) 46.42%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1532   +/-   ##
=======================================
  Coverage   46.41%   46.42%           
=======================================
  Files         564      564           
  Lines       35765    35775   +10     
  Branches     8946     8950    +4     
=======================================
+ Hits        16599    16607    +8     
- Misses      19116    19118    +2     
  Partials       50       50

Flag	Coverage Δ
unit	`46.42% <78.57%> (+<0.01%)`	⬆️

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

Files	Coverage Δ
packages/auth-plugins/src/LoginForm.tsx	`91.66% <ø> (ø)`
packages/code-studio/src/styleguide/Progress.tsx	`66.66% <ø> (ø)`
packages/code-studio/src/styleguide/Tooltips.tsx	`71.42% <ø> (ø)`
packages/components/src/LoadingSpinner.tsx	`80.00% <100.00%> (-20.00%)`	⬇️
...ges/components/src/context-actions/ContextMenu.tsx	`39.03% <ø> (ø)`
.../src/command-history/CommandHistoryItemTooltip.tsx	`83.33% <100.00%> (ø)`
...console-history/ConsoleHistoryResultInProgress.tsx	`66.66% <ø> (ø)`
packages/iris-grid/src/ColumnStatistics.tsx	`3.33% <ø> (ø)`
packages/iris-grid/src/IrisGridCopyHandler.tsx	`83.33% <100.00%> (+0.10%)`	⬆️
packages/iris-grid/src/PartitionSelectorSearch.tsx	`69.75% <ø> (ø)`
... and 5 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mofojed · 2023-09-27T15:24:56Z

Ah yea, using role="progressbar" makes sense, that would be better for tests to use than the data-testid. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/progressbar_role
Not all tests were using the data-testid so those failing now can be updated to look for that role instead, and/or you could have a utility function for checking if there's a loading element.

deephaven#1531

bmingles · 2023-09-27T22:30:46Z

@mofojed all tests have been updated, and e2e are passing as well.

dsmmcken reviewed Sep 21, 2023

View reviewed changes

bmingles commented Sep 21, 2023

View reviewed changes

bmingles mentioned this pull request Sep 25, 2023

EPIC: Theming #1537

Closed

13 tasks

mofojed requested changes Sep 25, 2023

View reviewed changes

bmingles force-pushed the 1531-loading-spinner-stutter branch from deef131 to 7898ba7 Compare September 25, 2023 21:42

bmingles changed the title ~~fix: CSS based loading spinner~~ fix!: CSS based loading spinner Sep 26, 2023

bmingles marked this pull request as ready for review September 26, 2023 15:54

bmingles requested review from dsmmcken and mofojed September 26, 2023 16:03

mofojed previously approved these changes Sep 27, 2023

View reviewed changes

bmingles dismissed mofojed’s stale review via 881a7a5 September 27, 2023 14:27

bmingles requested a review from mofojed September 27, 2023 15:11

bmingles added 14 commits September 27, 2023 17:03

CSS based loading spinner

e082aef

deephaven#1531

Semantic spinner colors

0c8ac73

deephaven#1531

Fixed invalid css variable

b6189d1

deephaven#1531

Disabled stylelint rule

299b38f

deephaven#1531

syncAnimationStartTime tests

814d658

deephaven#1531

Adjusted loading overlay styles

7596430

deephaven#1531

Added style comment

611d28d

deephaven#1531

Fixed failing unit tests

e37aacf

deephaven#1531

Small spinner vertical align

536fca7

deephaven#1531

Gap between loading spinner

9e81d0c

deephaven#1531

Fixed spinner alignment

ee0a93c

deephaven#1531

Default animation startTime + comments

a9c0f51

deephaven#1531

Renamed CSS class

79461e4

deephaven#1531

Vertical align inline spinners

9e1a8d6

deephaven#1531

bmingles added 6 commits September 27, 2023 17:03

Vertical align spinners

7ff5220

deephaven#1531

Moved loading spinner CSS class

017911f

deephaven#1531

Updated snapshots

4cf22b0

deephaven#1531

Ran prettier on scss files

7d42374

deephaven#1531

Added aria + role attribute. Fixed tests

fdd34ee

deephaven#1531

Fixed spinner tests

77480a8

deephaven#1531

bmingles force-pushed the 1531-loading-spinner-stutter branch from aeeb5ce to 77480a8 Compare September 27, 2023 22:03

mofojed approved these changes Sep 28, 2023

View reviewed changes

bmingles merged commit f06fbb0 into deephaven:main Sep 28, 2023

bmingles deleted the 1531-loading-spinner-stutter branch September 28, 2023 13:42

github-actions Bot locked and limited conversation to collaborators Sep 28, 2023

Conversation

bmingles commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mofojed left a comment

Choose a reason for hiding this comment

Uh oh!

mofojed commented Sep 27, 2023

Uh oh!

bmingles commented Sep 27, 2023

Uh oh!

codecov Bot commented Sep 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mofojed commented Sep 27, 2023

Uh oh!

bmingles commented Sep 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bmingles commented Sep 20, 2023 •

edited

Loading

codecov Bot commented Sep 27, 2023 •

edited

Loading