adds loading spinner when app is first loading#221
Open
will-weiss wants to merge 1 commit into
Open
Conversation
| <div class="app"></div> | ||
| <div class="app"> | ||
| <div class="authenticating"> | ||
| <div class="authenticating-spinner"> |
Contributor
There was a problem hiding this comment.
Can't we use CSS for the dark theme? Why do we need 2 files?
Contributor
There was a problem hiding this comment.
The spinner doesn't seem to spin 🤔
Contributor
|
I don't understand your suggestion for the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work in progress - the general idea is to add a loading spinner for the whole page while React is busy painting so that the user knows something is going on.
This view should take up the same view height as the rest of the extension, so I used a magic number of
388pxto do that. An issue is that the view when you're not on a webpage is an alert that should not be that high.The good news is that we should always know ahead of time whether you're in one view or the other and can use browser.browserAction.setPopup to change the view ahead of time so that when the user clicks the popup the proper view loads and there is no glitch where a 388px high view loads and then is quickly replaced by a smaller alert.
This approach feels a bit hacky as we're duplicating work to some extent by inlining the
Alertreact component and just making thenot-a-webpage.htmlbut I think it's a reasonable approach.Another thought that's coming to mind is what if the user uses dark mode. Currently that's not handled, so would we have 2
.htmlfiles? One for dark mode and one for light mode? At that point we may want to generate these HTML files from React rather than relying on copy/paste - which could get out of hand very fast.If we like this, we might also consider having using a similar approach for the
NotAuthedview when we know the user is not authenticated and render a static screen with a link. Then, in thepopup.html(which we might rename as there would be several htmls describing the popup) we could inline the buttons so that the loading spinner is just in the upper half and the user would immediately see the buttons.Thoughts?