Closed octopusinvitro closed 8 years ago
Looks awesome.. nice effort :)
Couple of comments:
1) Does the .gitignore need to include the jasmine and vendor folders? I can see that it avoids people setting this up locally having to run any build steps, but I wouldn't have thought a single npm install wouldn't be too much of a stretch, if that's possible in one command?
2) Little too many globals for my liking. In particular, Files
is a little nebulous. Could you maybe pass it as an argument to a collector function (Zipper.addFileSet()
?)
3) Seems like supporting multiple tutorial downloads might not be too complex to do.. If the download link was keyed (e.g. download:example1
) which followed through to another level in the files list at the top of the md and the all
could be replaced by the key from that new level. Then in src/ui.js, the selector checks for the start of the href attr starting with download:
, then your listener parses the href to return the key and pass it as an argument to Zipper.createZip(), where again you can just replace all
with the key that was passed in.
Thank you for taking the time to review the code, @kodikos. Great observations.
gulp-jasmine
in the gulp file and add a task to run the tests. I could add that and remove the jasmine folder, or keep both, so that everybody can choose how they want to run them? How do the rest of the contributors do it?body
, that would add 3 more http requests. It's a tradeoff, so whatever you all prefer is good for me. [*see footnote below][*] As far as I know, there is no CDN hosting the vendor libraries? The author seems to just link them from his GitHub account. I'm not sure if GitHub serves raw files with the right headers, although his examples work.
Overall, this looks like a big step in the right direction, and I'm inclined to get it merged and then iterate on it, rather than try to polish it in the PR.
Please update README.md with the relevant bits of instructions from the PR text.
I agree with Andrew, it's good enough to check in :+1: despite my comments for which you raise good points that suggest resolution in further PR's would be a better approach. I was going to try it out for you just to have an independent test of it, but haven't had the chance, might be able to sometime this week.
Just on point 2, I wasn't thinking of any hookup with frontmatter but just that it interacts directly with the JS classes that perform the task so there's no need for the Files
global.
@asuffield Thank you for the review. I have updated the README.
@kodikos It would be amazing if you could try it out before it is merged. :+1:
@kodikos Regarding Files
, what about this:
When the page is generated, instead of spitting its files in the JS, we could spit them in an HTML element that would be hidden for visitors, but accessible from the JS with a selector, so instead of this:
{% if page.files %}
{% assign pageurl = page.url | split: '/' | pop: | join: '/' %}
{% capture files %}[{% for file in page.files %}
"{{ pageurl }}/{{ file }}"{% if forloop.last %}{% else %},{% endif %}{% endfor %}
]{% endcapture %}
{% endif %}
<script>
var Files = {
all : {{ files }}
}
</script>
We would have something like this maybe:
{% if page.files %}
{% assign pageurl = page.url | split: '/' | pop: | join: '/' %}
<ul id="files" class="hidden">{% for file in page.files %}
<li>{{ pageurl }}/{{ file }}</li>{% endfor %}
</ul>
{% endif %}
(It's even shorter, he he :smile: My bad) I don't know how to make the frontmatter variable interact directly with the JS classes, because once the page is generated by Jekyll, the information in the frontmatter is lost for ever, so the JS can't access it in production, unless it is written to the page before, when it is being generated, as shown above.
What do you think?
Hi... that's a really good plan, I like the accessibility angle on it :) The user could just right-click and save as on them if JS was unavailable. Could make the list collapsable, too, to keep the page tidy (leave that for another change, tho, good exercise for a student!). Maybe we can build on that even further and be even better for accessibility, and jointly allowing you to link things up better by using an <a name="">
hashtag around the ul
(with a marker that it's a zippable thing), and use a normal in-page link, so that it's a real link again pointing to the list of files to be zipped. The only pre-prep would be an efficiency (again, a later student PR could do this) to avoid having a listener on every a
tag, just scan them for anchor tags that link to file lists (maybe a data-zippable="true"
tag on the anchor?) Then the Zipper just parses the file list at the time of generating the zip instead of building it up before.
Great points on accessibility @kodikos. :+1: :boom: :100:
Just to clarify for future developers coming to the code: At the moment there is not a listener for every a
tag. The listener is only added to download links:
var downloadLink = document.body.querySelector("a[href=download]");
if (downloadLink) {
registerListener(downloadLink);
}
I put the selector on the href
because Markdown doesn't understand ids or classes (unless you mix markdown and HTML syntax, but I wanted to keep it simple for tutorial creators).
Another thing is that the file list needs to be built up before (be it by storing it in a JS variable or in an HTML item list), since the frontmatter can't communicate directly with the JS in production.
We could stipulate that the anchor links to the zippable file list start with download
so that the selector can be 'a[href|="#download"]'
?
I understand that frontmatter communicates with JS using the medium of interpretive danceHTML. I just meant that you don't need to parse the HTML until the user actually clicks on the link to download it.
Hit a snag with testing as I was using an LXDE VM and it doesn't support Qt (so no node).. d'oh! Rectifying asap!
Hi... done my testing. No problems found (only tested FF)! I did find that you can pull any file from anywhere on the site into the zip, was that intentional? Not that it matters, the tutorial repo is on github anyway. I couldn't see a way to break out of it and reach other files, that's up to the security of jekyll. :+1:
@kodikos thanks for testing this, I wanted to test this myself as well but will not have time until at least next week. I do however trust your judgement so will merge it in :)
Thanks for reviewing as well @asuffield
Thanks for your work on this @octopusinvitro 👍 , great improvement to the tutorials 😄
This PR allows to version-control new and/or existing exercise files together with their tutorials. It generates a zip on the fly using JSZip. As an example of how to use it, it is applied to JS lesson3, as suggested by Sam.
How to use
To add downloadable files to a new or existing tutorial:
files
to the tutorial page with a list of the files you added, including folder name:download
:And you're done. Commit and push as usual.
Caveats:
At the moment this solution allows only ONE download link per page using this technique (the old links to the gists are still there). If this PR is approved I will update the liquid to make it work for several download links per page.
How it works
Liquid reads the variable in the frontmatter with the file names, and makes them available to the JS. Then the JS adds an event listener to links having an
href
ofdownload
that tells JSZip to generate the zip on the fly when the link is clicked. Files are downloaded asynchronously from the repo.Room for improvement
bundle exec jekyll serve --watch
running in the background will do. If one is to also make changes in the JS though,gulp
should be run in another terminal tab. Would be nice to have a task that takes care of that.Additions
The commits are self-explanatory. I added the JSZip libs, Jasmine for the tests, and a gulpfile to automate JS tasks. At the moment it only concat/minifies, and watches for changes. I also added a route to run the specs as well. So if you want to make changes to the JS, do this:
and then go to http://localhost:4000/test/specrunner.html to run the tests. Tests should be green.