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

Prevent render until all CSS is fulfilled #387

Merged
merged 1 commit into from
Jun 1, 2018

Conversation

Jaifroid
Copy link
Member

@Jaifroid Jaifroid commented Jun 1, 2018

@mossroy , I agree that this is a less risky way of solving (or rather, ameliorating) #381 for a v2.3 release.

I've implemented your helpful suggestions in commit 127ff29. Tested on IE, Edge, FFOS, Firefox Quantum.

I've also, optionally, added a commit 41f1b70 which displays an additional message to the user when the app is retrieving CSS from the ZIM instead of the cache. Because that is slow, and because first impressions often count strongly with apps, giving this additional small piece of feedback could be a user-friendly gesture. The message won't display again on subsequent page loads unless a further piece of CSS needs to be cached.

It's easy to drop that commit if you feel it's unnecessary. On FFOS, because of the very small screen size, it does look a bit cluttered the first time. But I think nowadays most mobiles have more width. However, the message could be abbreviated to just "Caching styles...". See what you think.

@Jaifroid Jaifroid added this to the v2.3 milestone Jun 1, 2018
@Jaifroid Jaifroid self-assigned this Jun 1, 2018
@Jaifroid Jaifroid requested a review from mossroy June 1, 2018 10:28
@Jaifroid
Copy link
Member Author

Jaifroid commented Jun 1, 2018

Maybe commit b985966 is a better way to display the progress message (it replaces the previous message rather than adding it on an extra line -- better for small screens?).

@mossroy
Copy link
Contributor

mossroy commented Jun 1, 2018

It looks like great job!
I've quickly tested on Firefox and a Firefox OS device, and it worked fine.
It's a good idea to add information about what's going on : it's certainly a relevant feedback for the user. I tend to prefer your first version (with the css message added to the original one) than the second (where it replaces it). It looks fine on my Firefox OS device too.

Copy link
Contributor

@mossroy mossroy left a comment

Choose a reason for hiding this comment

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

This looks good, and solves the issue in all the context and ZIM files I tested. On Firefox and Chrome, including browser extensions and ServiceWorker mode, and on Firefox OS devices

@Jaifroid Jaifroid force-pushed the Prevent-render-without-preload branch from 4f333c8 to 0be8cbb Compare June 1, 2018 13:57
@Jaifroid Jaifroid merged commit d1d9e49 into master Jun 1, 2018
@Jaifroid Jaifroid deleted the Prevent-render-without-preload branch June 1, 2018 14:03
@Jaifroid
Copy link
Member Author

Jaifroid commented Jun 1, 2018

Merged.

@mossroy
Copy link
Contributor

mossroy commented Jun 1, 2018

@ernesst : could you please test the ubuntu touch nightly build when it will be available in https://download.kiwix.org/nightly/2018-06-01/ ?
We're about to release version 2.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants