Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardizing spinner placement and adding missing spinners, Fixes #689 #3226

Merged
merged 8 commits into from
May 2, 2024

Conversation

alexcottner
Copy link
Contributor

@alexcottner alexcottner commented Apr 26, 2024

Goal of this PR is to add missing spinners and standardize spinner placement.

  • Fixes List of groups should show empty table while loading #689 by showing a spinner while groups are loading
  • If a spinner exists on a component using a tab control (ex: BucketTabs), spinner should exist inside of the tab control if we have enough context
  • If a spinner exists on a component that renders a table (ex: CollectionDataList), spinner should exist inside the table if we have enough context
  • Updated unit tests where adding or moving the spinner required a test update

@alexcottner alexcottner changed the title WIP - Standardizing spinner placement and adding missing spinners Standardizing spinner placement and adding missing spinners, Fixes #689 May 1, 2024
@alexcottner alexcottner marked this pull request as ready for review May 1, 2024 15:10
deleteBucket={handleDeleteBucket}
onSubmit={handleSubmit}
/>
{busy ? (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prettier, why do you like unnecessary parenthesis?

@@ -196,6 +194,8 @@ export default function SimpleReview({
);
};

console.warn(records);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftover

formData={formData}
onSubmit={onSubmit}
/>
{busy || formData == null ? (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here to explain why we have this condition on formData ?

Copy link
Contributor Author

@alexcottner alexcottner May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment, but it's just redux lifecycle quirkiness

} else if (
session.authenticating ||
session.busy ||
(records.loading && signoffSource && signoffDest)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall why we had this condition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we were just having one spinner for any of the conditions. In this case if we're waiting for auth, waiting for session info to load, waiting for the records to load, or waiting for the signoff info to load just show a spinner.

But now we've broken it up. If we have enough context to render the tabs (auth and session) we do so. And then we show a spinner under the tabs if we're waiting for the records or signoff info still.

@alexcottner alexcottner merged commit 3a0414e into master May 2, 2024
6 checks passed
@alexcottner alexcottner deleted the more-spinners branch May 2, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List of groups should show empty table while loading
2 participants