Add spinner to be shown before content is completely rendered #590

marvinchin · 2019-01-16T16:34:54Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] New feature

Resolves #484

What is the rationale for this request?

Time taken for the page to render causes flash of unstyled content. To avoid having the user see the partially rendered page, we instead show a spinner until content is ready.

What changes did you make? (Give an overview)

Added an overlay with a spinner to the generated page markup
Added method fadeOutSpinner to setup.js to remove the overlay when the page is done rendering
Updated the expected site generated to include the spinner markup and related assets

Example of spinner being shown (slow!)

Is there anything you'd like reviewers to focus on?

This spinner is shown briefly on every page load - will this be annoying to the user?
CSS for the spinner is taken from https://www.w3schools.com/howto/howto_css_loader.asp - do we need to attribute this in the source code?

Testing instructions:

Load any page. The spinner should appear while the page is rendering, and fade out when the page is done rendering.

yamgent

This spinner is shown briefly on every page load - will this be annoying to the user?

A good way to lessen the annoyance to the user would be:

To show the spinner only after the loading has taken more than a second.
To not fade out when the loading is complete (i.e. remove the fading, regardless of how long it is loading).

CSS for the spinner is taken from https://www.w3schools.com/howto/howto_css_loader.asp - do we need to attribute this in the source code?

Bootstrap already has their own spinner. Please use that one instead. Thanks!

yamgent · 2019-01-17T02:45:51Z

asset/css/markbind.css

+    background: white;
+    display: flex;
+    justify-content: center;
+    align-items: center;


Attributes should be listed in the alphabetical order.

asset/css/markbind.css

yamgent · 2019-01-17T02:47:33Z

asset/js/setup.js

@@ -33,10 +33,15 @@ function setupAnchors() {
  });
 }

+function fadeOutSpinner() {
+  jQuery('#spinner-overlay').fadeOut(100);


As stated earlier, this should be .remove() rather than fade out, to make it more pleasant for the user to navigate.

yamgent · 2019-01-17T02:48:52Z

src/Page.js

+
+Page.prototype.insertSpinner = function (pageData) {
+  return `${pageData}\n${SPINNER_HTML}`;
+};


The addition of the spinner need not be done through code, because the HTML content is always going to be the same.

Please add the spinner directly inside page.ejs instead.

damithc · 2019-01-17T03:05:26Z

Thanks for tackling this issue @marvinchin

As @yamgent the current implementation somehow gives the impression that the website is 'slow'. The main factor is there is a blank page (and a spinner) shown for a while; other websites don't usually do that.

Although this solution was suggested by me, now I wonder if the current status quo (i.e., showing partial content) is actually better than the solution (w.r.t. user perception)? The main problem with the current status is that the partially rendered menus look ugly/broken. Is there a way to fix that instead so that the partial rendering doesn't look so bad?

marvinchin · 2019-01-17T04:42:32Z

The main cause of the unstyled content appears to be waiting for the vue and vue-strap files to load. This delay is more prominent in dev environment as we aren't doing compression or caching (if this is where the original observation came from, this may be the reason). The delay doesn't seem too visible to me in production.

Users on a slow network might still face this problem. If this is a concern, we could take the approach suggested by @yamgent and show the spinner only if the assets have not been loaded after 1 second.

Happy to further discuss how else we can tackle this problem, or if we should handle it at all!

damithc · 2019-01-17T04:50:16Z

The delay doesn't seem too visible to me in production.

It depends on the site I think. e.g., this always show a broken page briefly when loading for the first time https://nus-cs2103-ay1819s2.github.io/cs2103-website/

marvinchin · 2019-01-17T05:00:23Z

That's true, first loads might always face this issue as the scripts aren't cached locally yet.

Do you have any thoughts about showing the spinner only after loading for some time then? We should also hide the content until it is fully rendered. This should avoid the user seeing the unstyled content without showing the spinner unless loading is taking too long.

damithc · 2019-01-17T05:04:36Z

That's true, first loads might always face this issue as the scripts aren't cached locally yet.

Try viewing a different page using the Schedule dropdown of https://nus-cs2103-ay1819s2.github.io/cs2103-website/. The problem happens in every page.

yamgent · 2019-01-17T06:22:07Z

The main problem with the current status is that the partially rendered menus look ugly/broken. Is there a way to fix that instead so that the partial rendering doesn't look so bad?

We can add temporary classes to the components, before vue-strap finishes loading and starts to replace the components with actual implementations.

In the case of a <navbar>, we can try to add Bootstrap navbar classes to it so that at least they are positioned correctly while vue-strap is loading:

$('navbar').addClass('navbar navbar-expand-md fixed-top');

marvinchin · 2019-01-17T07:35:35Z

Do you think we should hide the content entirely until the required assets are done loading? Or is there some other way we should style them? Transitioning from temporary styles to the fully rendered styles may look strange otherwise.

On the note of reducing the time taken to fully render the page, I've taken a closer look at the CS2103 website, and it seems like rendering is blocked until siteData.json is loaded. The problem seems to be that we are loading images etc. before siteData.json. As the siteData.json is used only for search and is not user facing, we might be able to load it asynchronously to avoid it blocking the render.

yamgent · 2019-01-18T06:08:12Z

Do you think we should hide the content entirely until the required assets are done loading? Or is there some other way we should style them? Transitioning from temporary styles to the fully rendered styles may look strange otherwise.

We can add on top of the styles I have mentioned with visibility: hidden; so that they stay hidden but the position of the content do not jump around too much.

I think it is fine to not hide the words, since the user will still be allowed to get a basic sense of the website's content, regardless of whether the style has finished rendering or not. When I visit websites, things that appear "hidden", only to show up later, are usually always ads. :P

marvinchin · 2019-01-18T06:11:43Z

Noted, will make a separate PR for these changes! Thanks for the inputs! 🙂

marvinchin · 2019-01-19T07:38:01Z

Hi @yamgent @damithc I've made the changes requested. To summarise, we 1. hide content until it is ready to be rendered fully, and 2. show a spinner if rendering is not completed after 1 second.

Please do let me know if this behaviour is acceptable or if I should make any other changes!

yamgent · 2019-01-20T03:39:49Z

src/template/page.ejs

-    <link rel="stylesheet" href="<%- asset.bootstrapVue %>">
-    <link rel="stylesheet" href="<%- asset.fontAwesome %>" >
-    <link rel="stylesheet" href="<%- asset.glyphicons %>" >
-    <link rel="stylesheet" href="<%- asset.highlight %>">


These should not be moved to the bottom:

The convention is to put stylesheets at the head.

Authors may need to override the stylesheets, and they currently do that by adding additional stylesheets in head. Moving it to the bottom prevents them from overriding the stylesheets.

The reason I moved them to the bottom is to reduce the time taken to start showing the spinner. Otherwise, we have to wait for the other much larger assets to load before the spinner is shown. On slower connections this delay can be several seconds.

If this issue is not a concern, I will remove these changes!

The main bottleneck of the loading process is vue-strap and bulky/embedded page content and assets (e.g. youtube videos). Stylesheets themselves are fine and do not usually take up most of the loading time.

Will remove these changes!

yamgent · 2019-01-20T03:42:55Z

src/template/page.ejs

    <link rel="stylesheet" href="<%- asset.markbind %>">
-    <link rel="stylesheet" href="<%- asset.layoutStyle %>">
-    <% if (siteNav) { %><link rel="stylesheet" href="<%- asset.siteNavCss %>"><% } %>


Same comment as previous

yamgent

My apologies, I missed this PR because I didn't realise that it has been updated.

marvinchin · 2019-01-26T03:36:03Z

Thanks for looking at it!

Also, just to update - @damithc wants to compare the difference in perceived load times between showing the spinner vs just showing the semi-rendered page. If showing the spinner makes the page load look slower, we may choose not include this feature. I'll keep you posted once he has decided whether or not to go ahead with this!

marvinchin · 2019-01-26T05:58:21Z

@damithc has given the go ahead to include this.

Thanks everyone for looking at this!

pyokagan · 2019-01-26T06:28:57Z

Does this mean the website will "forever be loading" if Javascript is disabled?

Not sure how I feel about that, the SE-EDU website currently works without Javascript so it'll be sad if the learning resources page can't be viewed just because the reader has javascript disabled :-/

marvinchin · 2019-01-26T08:41:14Z

@yamgent I believe we spoke about this and the decision was that we can assume our users use JavaScript. Do you think this use case is something we should account for?

If it is, we can modify the CSS and use some inline JavaScript to show this spinner only to users with JavaScript enabled.

pyokagan · 2019-01-26T08:52:17Z

afaik usually server side rendering is used to deal with the FOUC problem.

^{(Notice how https://se-edu.github.io/learningresources/ doesn't have any FOUC 😛 ) Scratch that, it has FOUC now after the update. Go look at an archive or something.}

yamgent · 2019-01-26T08:56:29Z

Vue, a major framework used by MarkBind, is using JavaScript. Website visitors can be assumed to be using JavaScript.

If the concern is that users with JavaScript disabled do not know why the web page is blank, we can add a warning message on the overlay to ask the user to enable JavaScript.

pyokagan · 2019-01-26T09:08:49Z

No, the concern is that this unnecessarily handicaps MarkBind and prevents it from being used in websites that do not need any of the flashy Javascript-required features.

https://vuepress.vuejs.org/ doesn't need the reader to have Javascript, why should MarkBind?

marvinchin · 2019-01-26T16:26:22Z

If supporting users who have javascript disabled is a concern, we may also need to think about how to make the other parts of the site work without javascript (e.g. dropdowns and search). In the meantime, we could make the overlay and spinner visible only on browsers with javascript enabled to avoid having the page be entirely unusable for these users using the approach I suggested above. Alternatively, if we don't foresee this enhancement bringing much value to users we could just shelve it for now.

SSR might be a possible approach to avoid FOUC, but I think that may require further investigation on how to integrate it into MarkBind. However, I'm not sure where that lies in the current priorities for the 2.0 release.

Just my $0.02!

yamgent · 2019-01-27T02:13:13Z

No, the concern is that this unnecessarily handicaps MarkBind and prevents it from being used in websites that do not need any of the flashy Javascript-required features.

Yes, you are right in that we can be more gentle to website that generally do not rely on dynamic content or dynamic features that MarkBind provides.

https://vuepress.vuejs.org/ doesn't need the reader to have Javascript, why should MarkBind?

Thanks for mentioning the vuepress project. I checked their project and I think their idea of using Vue as a server-side rendering, rather than client-side rendering, is pretty nice since it allows them to reduce their reliance on JavaScript to purely just dynamic features. This is certainty an idea we can consider exploring in the future, so that Vue and vue-strap no longer need to be bundled together with MarkBind websites.

@marvinchin

In the meantime, we could make the overlay and spinner visible only on browsers with javascript enabled to avoid having the page be entirely unusable for these users using the approach I suggested above.

Pardon me for forgetting if we discuss this before, but if we do it in this order, the un-rendered content will flash for a second before the overlay is enabled right? I don't think that will look very nice to the user.

Alternatively, if we don't foresee this enhancement bringing much value to users we could just shelve it for now.

Thanks for offering to shelve the enhancement, if we indeed end up doing so, do check with Prof to see if you can claim credit for the work you have done for this PR.

For the issue of the partial rendering temporary "breaking" the website, you can still consider the method that I have suggested earlier to make the website less ugly:

We can add temporary classes to the components, before vue-strap finishes loading and starts to replace the components with actual implementations.

In the case of a <navbar>, we can try to add Bootstrap navbar classes to it so that at least they are positioned correctly while vue-strap is loading:
$('navbar').addClass('navbar navbar-expand-md fixed-top');

damithc · 2019-01-27T02:25:57Z

Thanks for mentioning the vuepress project. I checked their project and I think their idea of using Vue as a server-side rendering, rather than client-side rendering, is pretty nice since it allows them to reduce their reliance on JavaScript to purely just dynamic features. This is certainty an idea we can consider exploring in the future, so that Vue and vue-strap no longer need to be bundled together with MarkBind websites.

It's worth investigating, especially if it can solve the FOUC probem, which is a pretty serious problem as it affect adoption.
If we don't apply it across MarkBind immediately, maybe we can consider using it for the TopNav, as that seems to me the main cause of FOUC at the moment?

yamgent · 2019-01-27T05:38:31Z

If we don't apply it across MarkBind immediately, maybe we can consider using it for the TopNav, as that seems to me the main cause of FOUC at the moment?

Sure, that seems like a reasonable start. @marvinchin interested?

marvinchin · 2019-01-27T06:09:54Z

Sure, I could look at that and hopefully have something up by this/next week!

Also, I've updated the PR to only show the loading overlay for users with javascript enabled, without having the flash of content before the spinner is shown (thanks @yamgent for pointing me in the right direction).

yamgent · 2019-01-31T01:46:10Z

@marvinchin there is a merge conflict with this PR, would need you to resolve it, thanks!

marvinchin · 2019-01-31T03:26:02Z

@yamgent fixed!

marvinchin changed the title ~~Add spinner to be shown before content is mounted~~ Add spinner to be shown before content is completely rendered Jan 16, 2019

yamgent requested changes Jan 17, 2019

View reviewed changes

marvinchin force-pushed the add-spinner branch 2 times, most recently from 0c191cd to 4c9afce Compare January 19, 2019 07:33

yamgent requested changes Jan 20, 2019

View reviewed changes

marvinchin force-pushed the add-spinner branch 2 times, most recently from 3355782 to d7f63cc Compare January 23, 2019 08:04

yamgent approved these changes Jan 26, 2019

View reviewed changes

yamgent added this to the v1.17.1 milestone Jan 26, 2019

marvinchin added the s.OnHold label Jan 26, 2019

yamgent removed this from the v1.17.1 milestone Jan 26, 2019

marvinchin removed the s.OnHold label Jan 26, 2019

marvinchin mentioned this pull request Jan 27, 2019

Use server side rendering for Vue components #637

Closed

yamgent approved these changes Jan 28, 2019

View reviewed changes

yamgent added this to the v1.17.2 milestone Jan 28, 2019

marvinchin force-pushed the add-spinner branch from 813ec6c to 1098687 Compare January 31, 2019 02:53

marvinchin added 7 commits January 31, 2019 11:00

Add spinner to be shown before content is mounted

d60cacf

Update expected site generated to include spinner

dabb414

Hide page content until render is complete

c4c2227

Fix PR comments

52aab46

Use overlay instead of inline styles

5d9e639

Update test files

55ab8a0

Hide loading overlay for users without javascript

c38023d

marvinchin force-pushed the add-spinner branch 2 times, most recently from 4bd8215 to 68bebb1 Compare January 31, 2019 03:18

Update tests

6ac5c1c

marvinchin force-pushed the add-spinner branch from 68bebb1 to 6ac5c1c Compare January 31, 2019 03:24

yamgent merged commit a721784 into MarkBind:master Jan 31, 2019

marvinchin deleted the add-spinner branch March 20, 2019 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add spinner to be shown before content is completely rendered #590

Add spinner to be shown before content is completely rendered #590

marvinchin commented Jan 16, 2019

yamgent left a comment

yamgent Jan 17, 2019

yamgent Jan 17, 2019

yamgent Jan 17, 2019

damithc commented Jan 17, 2019

marvinchin commented Jan 17, 2019

damithc commented Jan 17, 2019

marvinchin commented Jan 17, 2019

damithc commented Jan 17, 2019

yamgent commented Jan 17, 2019

marvinchin commented Jan 17, 2019

yamgent commented Jan 18, 2019

marvinchin commented Jan 18, 2019

marvinchin commented Jan 19, 2019

yamgent Jan 20, 2019

marvinchin Jan 20, 2019

yamgent Jan 20, 2019

marvinchin Jan 20, 2019

yamgent Jan 20, 2019

yamgent left a comment

marvinchin commented Jan 26, 2019

marvinchin commented Jan 26, 2019

pyokagan commented Jan 26, 2019

marvinchin commented Jan 26, 2019

pyokagan commented Jan 26, 2019 •

edited

yamgent commented Jan 26, 2019

pyokagan commented Jan 26, 2019

marvinchin commented Jan 26, 2019 •

edited

yamgent commented Jan 27, 2019

damithc commented Jan 27, 2019

yamgent commented Jan 27, 2019

marvinchin commented Jan 27, 2019

yamgent commented Jan 31, 2019

marvinchin commented Jan 31, 2019

Add spinner to be shown before content is completely rendered #590

Add spinner to be shown before content is completely rendered #590

Conversation

marvinchin commented Jan 16, 2019

yamgent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damithc commented Jan 17, 2019

marvinchin commented Jan 17, 2019

damithc commented Jan 17, 2019

marvinchin commented Jan 17, 2019

damithc commented Jan 17, 2019

yamgent commented Jan 17, 2019

marvinchin commented Jan 17, 2019

yamgent commented Jan 18, 2019

marvinchin commented Jan 18, 2019

marvinchin commented Jan 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamgent left a comment

Choose a reason for hiding this comment

marvinchin commented Jan 26, 2019

marvinchin commented Jan 26, 2019

pyokagan commented Jan 26, 2019

marvinchin commented Jan 26, 2019

pyokagan commented Jan 26, 2019 • edited

yamgent commented Jan 26, 2019

pyokagan commented Jan 26, 2019

marvinchin commented Jan 26, 2019 • edited

yamgent commented Jan 27, 2019

damithc commented Jan 27, 2019

yamgent commented Jan 27, 2019

marvinchin commented Jan 27, 2019

yamgent commented Jan 31, 2019

marvinchin commented Jan 31, 2019

pyokagan commented Jan 26, 2019 •

edited

marvinchin commented Jan 26, 2019 •

edited