re-organize install instructions

[fmichonneau]

This is a draft to attempt to move forward the conversation about the organization of the installation instructions on the workshop template.

It implements the solution initially proposed in carpentries/maintainer-RFCs#4 where the curriculum and flavor variables in the _config.yml file could be extended to accommodate combinations typically offered in SWC and LC workshops (in this PR only SWC workshops are being considered for now but if the solution is viable it could be extended to LC as well).

The installation instructions for each tool/software are moved into a separate sub-folder of _include which means that installation instructions could be shared across lesson programs (for instance, the git installation for SWC and LC workshops could be the same).

This file organization would make things easier for instructors setting up workshops website for the most typical cases and the simplified content of the _include/<lesson program>/setup.html makes it easier to toggle on or off manually which tool should appear on the workshop website.

While I'm generally in favor of having the installation instructions on each lesson, it creates an extra burden for learners to have to navigate to each of them to get the information they need.

@brownsarahm pointed me to remote include. This Jekyll plugin would allow us to have a single copy of the installation instructions that could then be displayed on both the workshop website and in the lessons. However, we can't use this plugin with a Jekyll-based site on GitHub Pages. If the instructions live in the workshop-template repository, then we could use this plugin by relying on a continuous delivery system (for instance GitHub Actions) on the lesson repository. We would use a jekyll build command to generate an HTML version of the lesson site, the remote includes would be fetched at build time, and we could push the site to the gh-pages branch.

While the other way around (having the installation in the lesson repo, and using remote include on the workshop website) seems more appropriate, I don't think it would work because:

1. using GitHub Actions (or another CD system) would require the instructors to set up a Github Personal Authentication Tokens to allow the action to push to their repo after they import the repository within their own account to set up the workshop website. In itself, this isn't too difficult but it's an extra step, has potential security risk if the tokens are not handled correctly.
2. (while small) it introduces a greater chance of "spoofing" where a bad actor could replace the remote include target with deceiving code while creating a fake workshop website. While there is also a chance it could happen on the lessons, it's far less likely given that the maintainers would be able to review pull requests that would affect these files.

I'd be curious to get the opinion of the @carpentries/lesson-infrastructure-committee and Lesson Maintainers on possible alternatives and path forward to address this.

References:

• see install instructions no longer rendered by default #660
• see Increasing modularity of install instructions **for SWC workshops** in workshop website template maintainer-RFCs#4

[netlify]

Deploy preview for zen-ride-08ffd1 ready!

Built with commit a29112e

https://deploy-preview-664--zen-ride-08ffd1.netlify.app

[zkamvar]

Overall, it looks good. The only concern I have is that it introduces a similar problem to #660, where the installation instructions default to being hidden on SWC sites.

What I would suggest instead, would be to issue a warning if the curriculum variable is not valid, but also include everything by default.

Removing the overall if statement and adding this at the top will work to accomplish that:

{% comment %}
Since curriculm could be expanded, this will loop through all valid values.

Params:
  - `curricula`: an array of valid curricula. Liquid doesn't really know how to
    create an array on the spot, so we are left with this hack from SO:
    https://stackoverflow.com/q/22763180/2752888. A better way to do this would
    be to add it as an entry in the _config.yml so that it can easily be
    expanded in the future when there are more SWC lessons.
  - `invalid_curriculum`: indicator to display the warning
{% endcomment %}

{% assign curricula = "swc-gapminder|swc-inflammation"  | split: "|" %}
{% assign invalid_curriculum = true %}

{% for curriculum in curricula %}
  {% if curriculum == site.curriculum %}
    {% assign invalid_curriculum = false %}
    {% break %}
  {% endif %}
{% endfor %}

{% if invalid_curriculum %}

{% include warning-curriculum.html %}

{% endif %}

{% include install_instructions/shell.html %}
{% include install_instructions/git.html %}
{% include install_instructions/editor.html %}

...

[ErinBecker]

I would particularly like to get feedback from @sstevens2 @munkm and @brownsarahm, as this proposed solution is intended to avoid problems like swcarpentry/git-novice#735 and #660. 🙂

[ErinBecker]

@fmichonneau - this looks like a good solution to me.

[brownsarahm]

Overall I like it. I think if this was added to styles/carpentries theme it might be better for maintainance so that lessons could stop relying on the rendered copy of the template.

Minor: why are there both html and md copies of shell instructions?

[fmichonneau]

Minor: why are there both html and md copies of shell instructions?

lack of sleep 💤. I removed the extra file. Thank you for spotting this!

As for the other comments, we'll respond in the next few days after we have a chance to get more feedback.

[fmichonneau]

@carpentries/lesson-infrastructure-committee we'd appreciate your comments and feedback on this pull request. Thank you!

[sstevens2]

I like the flexibility to have different install directions showing up and having the install tools outside of the lc/swc/dc structure in _includes.
I'm not sure I completely understand the part about using the remote include. If I understand correctly, the instructions would live in the template but then the lessons would use remote include to show the install directions in their setup page?

[maxim-belkin]

I found a few issues but overall it looks good. I'd be happy to re-review once it's updated.

[maxim-belkin]

OS X -> macOS
"10.9 or higher" is too vague because 10.9 was released in 2013. I believe that in recent macOS versions one has to install Command Line Tools (the process for doing that differs on different versions of macOS)

[fmichonneau]

I updated the OS X references but also now realizes that the recommended git osx installer hasn't been updated in over a year and I'm wondering if it's still the best recommendation for workshop participants 🤔

[maxim-belkin]

A few stylistic suggestions.

[maxim-belkin]

we can embed these videos....

[maxim-belkin]

After this change, we will see the above warning on this website.

[zkamvar]

For what it's worth, we currently already see an alert on the site, so another alert below it will not be too detrimental to the current status quo of the site.

The question becomes: who is the audience for https://carpentries.github.io/workshop-template? I know at the moment, several lessons point to the install page for updated installation instructions, but am I wrong in thinking that learners would not be visiting this site otherwise? Is there a better way to fail fast, early, and obviously so that the facilitators can set up the website properly?

[maxim-belkin]

We can hide divs that belong to class alert and alert-danger using jQuery:

<script src="https://code.jquery.com/jquery-3.5.0.js"></script>
<script>
$( document ).ready(function() {
  if (location.href.startsWith("https://carpentries.github.io/workshop-template/")) {
      $("div.alert.alert-danger").hide();
  }
});
</script>

[zkamvar]

We can hide divs that belong to class alert and alert-danger using jQuery

If we didn't want the alerts to appear on the page, we could just not place them there in the first place.

To determine if we should have alerts like this on the page, we have to ask: who is the target audience and how does it help us to have the alerts?

[maxim-belkin]

The alerts have to appear on the workshop (event) page if preparer forgot to update the fields but they don't have to appear on the workshop-template's website.

[maxim-belkin]

class "active" is added to articles when user clicks on corresponding <li> element. We shouldn't be adding them to all articles by default. This is not observed here, but if instead of "include"-ing these HTML files we were to add their contents, we'd see contents of all "active" articles. In the suggestions below, I kept class "active" only in the first article in a file.

[ErinBecker]

Hey, I just wanted to let you all know that I do plan to review this, I'm just behind on some other stuff. I'm hoping this coming week I'll have some time for a thorough review.

@munkm - Thanks so much for offering to review. @fmichonneau is working on incorporating the detailed feedback from @maxim-belkin (thank you Maxim!) and we're looking at finalizing this on Friday. Do you think you'd be able to look this over by the end of the week?

[maxim-belkin]

A few more suggestions / comments.

[maxim-belkin]

We might be able to detect this with jQuery.

$(document).ready(function(){
  if ($('iframe').height() == 0) {
  $('iframe').before('<p>Looks like your adblocker blocked registration window. Please navigate to <a href="https://www.eventbrite.com/tickets-external?eid={{page.eventbrite}}&ref=etckt">https://www.eventbrite.com/tickets-external?eid={{page.eventbrite}}&ref=etckt</a> to register.</p>');
  }
});

[maxim-belkin]

Looks great! Minor HTML clean-ups.

[fmichonneau]

Thank you for your comments and review! especially Maxim for spotting all the small but important details.
@munkm if you have comments about these changes, feel free to open a new issue/pull request or add more comments here.