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.

Sign up for GitHub

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

Jump to bottom

Prevent full page spinner from flashing on load #795

Merged
merged 25 commits into from
Oct 20, 2023
Merged

Prevent full page spinner from flashing on load #795

merged 25 commits into from
Oct 20, 2023

Conversation

travjenkins
Copy link
Member

@travjenkins travjenkins commented Oct 18, 2023 •
edited
Loading

Issues

Long standing issue

Changes

OnLoadSpinner

  • Create a context that can show a full page spinner while the app starts
  • Allow default value to be set so OAuth can be extra safe and not use this
  • Added in component that will turn off/on the spinner
  • Added the "turning off" component to full page wrapper and app layout as that seems to cover all the pages that can load while logging in.

Guards

  • The guards no longer need to show their own spinner (except grants) so they now return null during load
  • Removed some suspense uses as it was not doing anything right now

FullPageSpinner

  • Have the full page spinner no longer show right away to give the network a second and not always flash the indicator to people
  • Allow a delay to be passed in in case we need to skip the wait / make it longer

index.html

  • Cleaned up some comments. Considered defaulting the background color but since we dark and light mode this would not always make the app look like it is loading faster.

Tests

Manually tested

Both new and existing users...

  • Logging in
  • Granting tenant access (logged in and logging in)
  • Granting tenant access with fake token
  • Granting tenant access with used token
  • Google Sheets OAuth

Screenshots

Kind of hard to snag a screenshot and don't wanna include a video. Basically when the app is loading you should see one fullpage spinner during login

@travjenkins
saving off work so far
fb963b1
@travjenkins
Stop showing full page spinners to reduce flashing.
40eec31 
Use a new component that controls a full page spinner on on load
@travjenkins
Saving off work so far
a5a0502
@travjenkins
Matching favicon from the marketing site
ac2bd8b
@travjenkins
Removing some comments that aren't needed
5ba5f8d 
Adding a type to the icon
@travjenkins
Adding a single spinner to show during onload of the main app
c67fc9f
@travjenkins
Adding a single spinner to show during onload of the main app : new file
49b84d3
@travjenkins
Do not have the full page spinner be visible right away
366f319
@travjenkins
Allow some pages to not show the spinner (mainly for OAuth)
148d933
@travjenkins
Using an effect to make more flexible and to reduce change it gets st…
fe235b1 
…uck "on"
@travjenkins
Allowing a delay to be passed for when we want to show something righ…
31aaa6a 
…t away
@travjenkins
Allowing a default state for the provider so we don't need extra nesting
9c4cb42
@travjenkins
Having all OAuth related stuff show the spinner right away
1b1239d 
Also hiding the spinner for the OAuth related popup just in case as it shows its own spinner
@travjenkins
On the off chance the processor hangs making the header display
9213ca9
@travjenkins
Reverting back to reduce any chance of issues
5f67f11
@travjenkins
pr commenting
1dfd4c6
@travjenkins
Moving where this is set one layer higher as that seems to make more …
19a4a41 
…sense
@travjenkins travjenkins marked this pull request as ready for review October 19, 2023 02:30
@travjenkins travjenkins requested a review from a team as a code owner October 19, 2023 02:30
kiahna-tucker
kiahna-tucker approved these changes Oct 20, 2023
View reviewed changes
src/context/OnLoadSpinner/OnLoadSpinner.tsx Outdated
function OnLoadSpinner({ children, display }: Props) {
const { setLoading } = useOnLoadSpinner();

console.log('OnLoadSpinner', display);
Copy link
Member

Choose a reason for hiding this comment

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

Flagging the presence of a logging statement in the event it should be removed.

@travjenkins travjenkins merged commit 36517db into main Oct 20, 2023
@travjenkins travjenkins deleted the travjenkins/stopLoginFlashing branch October 20, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kiahna-tucker kiahna-tucker kiahna-tucker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@travjenkins @kiahna-tucker