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

Display a loading message instead of blank page #386

Merged
merged 1 commit into from
Jun 19, 2016
Merged

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Jun 7, 2016

07-1755-f271e11

@xPaw xPaw added Type: Feature Tickets that describe a desired feature or PRs that add them to the project. second review needed labels Jun 7, 2016
@xPaw xPaw added this to the 2.0.0 milestone Jun 7, 2016
@astorije astorije self-assigned this Jun 7, 2016
@maxpoulin64
Copy link
Member

I'm not sure about this one. I think having a loading screen is a good idea, but it would be nice if it reflected the correct status so it doesn't talk about Javascript and connectivity issues when it's fine. Even on localhost it takes a few seconds to load, and it's not rare for it to take 10-15 seconds on mobile to load. I don't think we can have the progress, but just displaying what it's doing would be great.

@xPaw
Copy link
Member Author

xPaw commented Jun 12, 2016

@maxpoulin64 Is that better? It will now say Connecting and Authorizing while hiding the message about javascript as soon as it loads.

@astorije
Copy link
Member

I'll try this before giving a +1.

@maxpoulin64
Copy link
Member

Love it, 👍

@xPaw xPaw force-pushed the xpaw/loader branch 2 times, most recently from 1165dd0 to b50dab1 Compare June 19, 2016 13:01
@xPaw
Copy link
Member Author

xPaw commented Jun 19, 2016

I've rebased this and also fixed colors in morning/zenburn themes.

@raqbit
Copy link
Contributor

raqbit commented Jun 19, 2016

It works. Cool :)

@astorije
Copy link
Member

This is really cool!
A few suggestions/comments before I'm a +1:

  • I'm not sure about that You must use a modern browser with JavaScript enabled. statement. If it stays on for too long, it suggests the user that they do not have a compatible browser. At the very least, I would change it for Make sure to use a modern browser with JavaScript enabled. so that it shows we do not check that ourselves, but personally I'd rather ditch it entirely.
  • What about a timeout to display If this message still does not go away, there might be connectivity issues.? There is no need to display it the first, say 5 seconds where this is loading so having it appear only after some time makes sense to me.
  • You're not connected to any networks yet displays when the page is loading, which has always bugged me. This would be a good time to hide it until the page is loaded (and the user is authenticated in private mode 1).

1 It would actually be nicer to disable the sidebar menu on the login page, but that's a different PR :-)

@xPaw xPaw force-pushed the xpaw/loader branch 3 times, most recently from f6492d6 to 276dfd3 Compare June 19, 2016 17:31
</div>
</div>
</div>
<script async src="js/loader.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Why async here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This script is not important enough to block for it.

Copy link
Member

Choose a reason for hiding this comment

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

Would call it loading-slow-check.js or something like that, unless you are planning to add more loader business there.

@astorije
Copy link
Member

This is great, 👍 and merging!

You're not connected to any networks yet displays when the page is loading, which has always bugged me. This would be a good time to hide it until the page is loaded (and the user is authenticated in private mode1).

Not addressed but can definitely be part of a further PR, this is already a great step ;-)

@astorije astorije merged commit 4b90178 into master Jun 19, 2016
@astorije astorije deleted the xpaw/loader branch June 19, 2016 18:17
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
Display a loading message instead of blank page
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants