Enable multiple zips per tutorial page #298

kodikos · 2016-07-14T21:43:03Z

What it does

This adds the ability to specify multiple file sets of files in the tutorials repo for insertion into zip files that can be offered up from a single page. It also addresses accessibility by allowing the individual files to be accessed via links, but makes it clear which bundle they are in.

How it does it

The link is now an anchor that points to a location at the top of the page that has a list of links to the files to include in a zip. The list is hidden when JS is enabled and zips can be generated. The JS adds an event to all links whose href starts with #download-. The JS reads the file list next to the corresponding anchor and builds the zip.

How to do it

Some changes are required to the file list. Firstly, to add a level that contains an identifier for the zip name that also forms the anchor name used in the link. Secondly, that level is a key/value pair to overcome limitations with liquid's variable handling (weird associative array support). E.g.

files:
  - zipname: example1
    files: 
    - files/index.html
    - files/jquery.js
  - zipname: example2
    files:
    - files/script.js
    - files/style.css


          Starting to have a stab at improvements


          WIP: building up file list location


          JS now supports accessible multiple downloads per page


          Got mutliple zips basically working

kodikos · 2016-07-14T21:44:34Z

This is still a WIP, I know it needs some tidying. Just offering it up as an RFC.

octopusinvitro · 2016-07-17T14:29:21Z

_layouts/page.html

+
+    {% if page.files %}
+      {% for zip in page.files %}
+        <a class="tutorial-filelist__heading" name="download-{{zip.zipname}}">Files for {{zip.zipname}}</a>


The name attribute is obsolete in a tags (not in form elements, though).
You can use id="download-{{zip.zipname}}" instead.

Also, I personally wouldn't use an a element here, but a heading, so that there is an indication that this is a different section from what is strictly the tutorial copy. So something like, either:

<h2 class="tutorial-filelist__heading" id="download-{{zip.zipname}}">Files for {{zip.zipname}}</h2>

or maybe:

<section> <h1 class="tutorial-filelist__heading" id="download-{{zip.zipname}}">Files for {{zip.zipname}}</h1> </section>

Or even:

<section> <h1>Download the tutorial files!</h1> <p class="tutorial-filelist__heading" id="download-{{zip.zipname}}">Files for {{zip.zipname}}</p>  <p class="tutorial-filelist__heading" id="download-{{zip.zipname}}">Files for {{zip.zipname}}</p>  </section>

where zip.zipname is the corresponding name.

octopusinvitro · 2016-07-17T14:38:07Z

javascripts-dev/src/ui.js

  };

  var registerListener = function(link) {
+    console.log("adding listener to", link);


I personally wouldn't include debugging code in commits that may end up in production, but that's maybe a personal choice.

You can use git add -p to granularly exclude the lines that you don't want to add to the commit.

octopusinvitro · 2016-07-17T14:40:10Z

javascripts-dev/src/ui.js

    link.addEventListener("click", function(event) {
      event.preventDefault();
-      createZip();
+
+      //  Get the file list on the fly


I also usually don't commit comments... again, probably just a personal choice.

octopusinvitro · 2016-07-17T14:47:14Z

stylesheets/style.css

+
+.hidden {
+	display: none;
+	visibility: hidden;


display:none is not very accessible. I personally prefer the more tested way used in the HTML5 boilerplate (the .visuallyhidden class) to hide elements visually.

But I do want to hide it from screen readers if the JS is available because the zip links will work and are accessible, right?

octopusinvitro · 2016-07-17T15:03:52Z

js/lesson3/tutorial.md

@@ -46,7 +50,7 @@ Using jQuery and JavaScript functions, we are going to build a small
 todo list.

 Download the files that you will need to work through the example
-[here](download).
+[here](#download-example1). and [example2](#download-example2)


"Download the files that you will need to work through the example here. and example2" doesn't read very well to me.

But I would only download the files for Exercise 1 in the section for exercise 1. The files for exercise 2 should be downloaded from exercise 2.

Also, Exercise 2 still has a link in "Download the files required to begin working through the example." pointing to the gist file.

This is only the test illustration, if you notice the file list is the exercise 1 zip split in 2

octopusinvitro · 2016-07-17T15:55:58Z

Skimmed over it and made some tiny comments, I'm running out of time now but will come back to it to review it properly and more thoroughly at some point during this week or next weekend.

Thank you for working on it! 👍

kodikos · 2016-07-17T22:50:35Z

Great, thanks for having a look :) I'm already into taking on most of your suggestions. As I say, this is bit of a WIP - was looking to make sure the proof of concept was good before putting more effort on final touches to make it prod ready (hence not wanting to PR it because then it gets full review treatment).
Btw, that also means you should run the gulp task, as I don't think the dl bundle is up to date!

octopusinvitro · 2016-08-13T00:28:35Z

Sorry I've been busy starting a new job and moving to a new city and couldn't find the time to review this properly.

I ran Jekyll and if I click a download link after I have already pressed another one, it adds the files of the previous zip to the current one, in the same zip. May be a bug that can be solved once you have finished polishing.

I tried to reason about the ui class and found it a bit confusing. I think it would be good for both you and your reviewers to maybe split this PR in two, one to add accessibility, and another to add multi-zip downloads. That way we can handle the separation of concerns better.

Nevertheless, I believe the code can still be simplified a bit. Thinking about the tutorial creators, I would replace:

files:
  - zipname: example1
    files:
    - files/index.html
    - files/jquery.js
  - zipname: example2
    files:
    - files/script.js
    - files/style.css

and the folder structure:

lesson3/
  files/
    // all the files mixed
  tutorial.md

with something simpler like:

files:
  - example1:
    - index.html
    - jquery.js
  - example2:
    - script.js
    - style.css

So that we can have a folder structure like:

lesson3/
  example1/
    // files for example1
  example2/
    // files for example2
  tutorial.ms

Having all the files for different tutorials mixed in the same folder doesn't seem like a good idea. For example, it's very common that they all have an index.html file. Having them in separate folders would also make it easier to drop folders in there for every new example. In the page template, in liquid, you can access a hash key with [0] and a hash value with [1]. That would simplify things a bit.

I also think removing the a tag and placing an id="download-etc" in the ul directly may simplify accessing the element and the code in the UI.

As I said, I think it would be desirable to break this PR into two different concerns to make it easier to reason about the code.

Again, thank you for working on it.

kodikos added 4 commits May 31, 2016

Starting to have a stab at improvements

d877791

WIP: building up file list location

7bbf1f8

JS now supports accessible multiple downloads per page

8d02e28

Got mutliple zips basically working

594a2da

octopusinvitro reviewed Jul 17, 2016
View changes

codebar / tutorials

Enable multiple zips per tutorial page #298

Enable multiple zips per tutorial page #298

kodikos commented Jul 14, 2016 •

edited

kodikos commented Jul 14, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

octopusinvitro commented Jul 17, 2016 •

edited

kodikos commented Jul 17, 2016

octopusinvitro commented Aug 13, 2016 •

edited

codebar / tutorials

Sponsor codebar/tutorials

Join GitHub today

Enable multiple zips per tutorial page #298

Enable multiple zips per tutorial page #298

Conversation

kodikos commented Jul 14, 2016 • edited

What it does

How it does it

How to do it

kodikos commented Jul 14, 2016

This comment has been minimized.

octopusinvitro Jul 17, 2016 • edited Member

This comment has been minimized.

octopusinvitro Jul 17, 2016 • edited Member

This comment has been minimized.

octopusinvitro Jul 17, 2016 Member

This comment has been minimized.

octopusinvitro Jul 17, 2016 • edited Member

This comment has been minimized.

kodikos Jul 17, 2016 Author

This comment has been minimized.

octopusinvitro Jul 17, 2016 Member

This comment has been minimized.

kodikos Jul 17, 2016 Author

octopusinvitro commented Jul 17, 2016 • edited

kodikos commented Jul 17, 2016

octopusinvitro commented Aug 13, 2016 • edited

kodikos commented Jul 14, 2016 •

edited

octopusinvitro Jul 17, 2016 •

edited

Member

octopusinvitro Jul 17, 2016 •

edited

Member

octopusinvitro Jul 17, 2016
Member

octopusinvitro Jul 17, 2016 •

edited

Member

kodikos Jul 17, 2016
Author

octopusinvitro Jul 17, 2016
Member

kodikos Jul 17, 2016
Author

octopusinvitro commented Jul 17, 2016 •

edited

octopusinvitro commented Aug 13, 2016 •

edited