-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add initial loading-screen to index.html #1825
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'm not seeing the loading screen... 🤔
Notice the blank white page after refresh is clicked (this in FFDev):
loading.missing.mov
Passing run #1847 ↗︎
Details:
Review all test suite changes for PR #1825 ↗︎ |
@taoeffect I see. |
I'll reply on Slack |
@taoeffect |
frontend/main.js
Outdated
const loadingScreenEl = document.querySelector('#main-loading-screen') | ||
if (loadingScreenEl) { | ||
loadingScreenEl.classList.add('is-removed') | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will minimize the element so that it becomes a single pixel wide. Why couldn't it be removed instead, or its display set to none
? I think a clarifying comment would be useful here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snowteamer I think it makes sense to remove it completely instead of hiding it if there is no other cases that the app needs to display this loader screen.
@taoeffect
Hope that looks good for you too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting approach @SebinSong!
When I loaded a previously loaded session (after starting FFDev), I was greeted with the following:
So there are 2 bugs here:
- Vue correctly complains about
<loading>
component not being defined as part of thecomponents
ofApp.vue
inmain.js
- The loading screen covers this prompt that is supposed to appear when quitting
grunt dev
and restarting it while there was an existing user+group:
To reproduce, create a group, then restart grunt dev
and refresh the page after grunt dev
fully loads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @SebinSong - seems like all of the issue were fixed!
Only thing is, some debug console.log
statements were still left in the PR. Once those are removed I can merge!
@taoeffect my bad on all those |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done @SebinSong!
With some more testing I noticed an issue I didn't notice before.
If you perform a refresh it always redirects to the dashboard. For example, if you are viewing the general chatroom and refresh, it takes you to the dashboard still.
However, this seems to be an issue on master
too, not just this PR, so I'll go ahead and approve and then open a new issue.
closes #1822
@taoeffect
I thought I would try using the existing screen in the app, which is
Loading.vue
with adding a GI logo in it like below screenshot. The logo will only be exposed there when the component is used as the initial loading screen btw.If you have a more interesting idea for it though, let me know. happy to work on that.