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

Update the account screen loading state #41481

Merged
merged 1 commit into from
May 15, 2024
Merged

Conversation

bl-nero
Copy link
Contributor

@bl-nero bl-nero commented May 13, 2024

  • Don't show state pills until MFA devices are loaded
  • Actually show the spinner on the passkey list
  • Fix the test flakiness

Fixes #41402

Screenshot 2024-05-13 at 19 22 22

- Don't show state pills until MFA devices are loaded
- Actually show the spinner on the passkey list
- Fix the test flakiness
@bl-nero bl-nero added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 labels May 13, 2024
Comment on lines +367 to +376
<RelativeBox>
{fetchDevicesAttempt.status === 'processing' && (
// This trick allows us to maintain center alignment of the button
// and display it along with the indicator.
<BoxToTheRight mr={3} data-testid="indicator-wrapper">
<Indicator size={40} />
</BoxToTheRight>
)}
{button}
</RelativeBox>
Copy link
Contributor

Choose a reason for hiding this comment

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

is this how we are going to display spinners for buttons now? i always thought the spinner be inside the button but 🤷‍♀️

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 that a spinner inside a button is supposed to indicate that an operation performed by the button is pending. In this case, we are talking about a loading operation that triggers the button's availability.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah that makes sense 👍

@bl-nero bl-nero requested a review from ravicious May 14, 2024 09:04
@bl-nero
Copy link
Contributor Author

bl-nero commented May 14, 2024

One thing to note: This PR requires https://github.com/gravitational/teleport.e/pull/4128 to be merged first.

@bl-nero
Copy link
Contributor Author

bl-nero commented May 15, 2024

Tested: loading the account settings screen with network throttling in the following scenarios:

  • OSS, with passkeys
  • OSS, without passkeys
  • Enterprise, with passkeys

@bl-nero bl-nero added this pull request to the merge queue May 15, 2024
Merged via the queue into master with commit 7c55bc3 May 15, 2024
39 checks passed
@bl-nero bl-nero deleted the bl-nero/account-loading-state branch May 15, 2024 14:17
@public-teleport-github-review-bot

@bl-nero See the table below for backport results.

Branch Result
branch/v15 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v15 no-changelog Indicates that a PR does not require a changelog entry size/sm ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Account.test.tsx flakiness
3 participants