Skip to content
This repository has been archived by the owner on Apr 19, 2021. It is now read-only.

remove fireWhenReady timeout, and use script loaded event #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kevcenteno
Copy link

Problem

Used a timeout to check if all the scripts were loaded.

Solution

Used a 'load' event and a count to check if the all the scripts were loaded, then fires the markdown to html conversion.

@xeoncross
Copy link
Owner

This looks like a great change, my only concern is I think I ran across some browsers which did not have a "load" event for script tags. I know Firefox doesn't have a "load" event for style sheets (which is also something I would want to wait for).

I'm going to look into this again as soon as I can.

@kevcenteno
Copy link
Author

Ah true! From the quick research I've done, this event was added to HTML5; the fallback being using a timeout (ie what you had before).

My suggestion would be to concatenate showdown.js into jr.js, since it's a hard dependency.

@xeoncross
Copy link
Owner

@kevcenteno, I kept showdown out of jr.js so that jr would load faster and hide the content sooner. If we do some kind of * { display: none} rule or something on each page then it would be a good idea to merge them into the same javascript file.

@kevcenteno
Copy link
Author

@xeoncross I wouldn't use css to hide the content, in case the JS fails. Combining jr.js and showdown results in a 20kb file... which isn't that bad for a first request.

@xeoncross
Copy link
Owner

I just realized that both with your change, and the timeout I had we should only wait a max time before we assume there was a 404 or something and default back to showing the text anyway.

Also, I'm not totally sure about combining the two files yet, on one hand the system would load faster, on the other hand, it would take longer to hide the unstyled content as the larger file loaded.

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

Successfully merging this pull request may close these issues.

2 participants