carpentries / styles

Styles for The Carpentries lessons. No README to avoid merge conflicts with lessons. Demo 👇
https://carpentries.github.io/lesson-example
Other
84 stars 94 forks source link

Replace relative_root_path with |relative_url or site.baseurl #569

Closed unode closed 3 years ago

unode commented 3 years ago

In order to remove relative_root_path two substitutions were performed:

  1. {{ relative_root_path }} some/path became {{ 'some/path' | relative_url }}. 1.1. the special case {{ relative_root_path }}/ becomes {{ site.baseurl }}.
  2. {{ relative_root_path }}{% link some/resource.md %} became {{ site.baseurl }}{% link some/resource.md %}

The change in 2. will need to be adjusted when migrating to Jekyll 4.0 where {{ site.baseurl }} is already included by default with {% link %}.

Also removes _includes/base_path.html as this file is no longer necessary.

Fixes #568

@tobyhodges

unode commented 3 years ago

Looks like "R Intro Geospatial" is using _includes/base_path.html directly.

How should that be handled here?

unode commented 3 years ago

Pull request submitted to "R Intro Geospatial" lesson.


On a backwards compatible note. I realize that removing _includes/base_path.html can break other lessons if they are using the relative_root_path workaround.

I see two possibilities:

A. Breaking lessons is acceptable. Maintainers will be forced to modify lessons accordingly. B. Backwards compatibility is preferred. Keep _includes/base_path.html but replace its content with:

{% assign relative_root_path = site.baseurl %}

Both options should fix the underlying issue but B will keep an unnecessary, possibly confusing, variable around.

Which of the two options is preferred?

fmichonneau commented 3 years ago

Hi @unode,

Thanks for this PR, but unfortunately I think we need to keep using relative_root_path.

While your approach is a lot more elegant, it does require having the Jekyll server running to have a functional site. We need to be able to generate the site to work as a collection of static files (i.e., generated by the jekyll build command). But because we don't use the {site.baseurl} for our lessons, the generated files have paths that start with / breaking the styling.

The convoluted workaround with using relative_root_path has been the only solution so far (but please let me know if you can think of a better approach) that allows us to reliably provide working paths for both the output of jekyll serve and jekyll build.

unode commented 3 years ago

Hi @fmichonneau

I was not aware that having the entire site working through the file:// protocol was among the requirements. Apologies for this. The PR was indeed done under the assumption that GitHub Pages, jekyll serve or a simple HTTP server (e.g. python3 -m http.serve) would be used. I see now why the approach in this PR wouldn't work on its own...

To clarify, this change was motivated by an issue (#568) affecting pages that can serve the same layout from different URLs, effectively breaking the relative_root_path approach.

While looking for alternatives I also found a PR on relativize_url that ended up dismissed. It seems like this would be desirable here but I'm not sure it would solve #568 . It wouldn't work for {% link ... %} either since filters aren't allowed in this context.

I see only two ways to address all the above. Both need to be context aware: A. Implement the logic in jekyll - (relativize_url + {% relativized_link ... %}?) B. Create a post-processing script that transforms /urls into ../../urls for every generated .html file.

B seems like the easiest and most maintainable solution. Could be run on demand to address the file:// use-case.

Thoughts?

PS: There's https://github.com/carpentries/styles/blame/gh-pages/bin/boilerplate/_extras/figures.md#L61 . I'm not sure how this JavaScript code is used...

unode commented 3 years ago

Needs a bit more testing to ensure all is working as expected but would you mind checking if this approach (updated PR) is reasonable and addresses all the discussion points above?

The bin/convert_urls_to_relative.py script that transforms the URLs can get rather verbose due to the /edit and /blob GitHub links in the footer. If the URL does not match a file on disk, a warning is issued but the URL is not modified. On non-GitHub builds repo_url = "" causes those links to render with a / prefix instead of a full URL. Should these prefixes be special-cased and ignored?

maxim-belkin commented 3 years ago

I sense that requiring Python to build a website locally isn't something maintainers will be thrilled about.

The 404 page comes into play when the website is served online or locally via jekyll serve. The problem is that it's generated in the very beginning and the same page is served regardless of whether the typo is website/lesson/typo.html or website/lesson/episodes/typo.html. The first page is displayed correctly because relative paths for this page are identical to those for 404.html. The second page is displayed incorrectly because relative paths are wrong -- they're missing the leading ../.

I submitted #578 that sets relative_root_path depending on whether we're building the website locally or using GitHub Pages.

unode commented 3 years ago

Thanks @maxim-belkin Python was used only due to my lack of familiarity with Ruby. Assumed would be an ok language since other .py scripts already exist in the repo.

578 looks like a simpler solution. I think we can close this one if the other addresses the same use-case.

maxim-belkin commented 3 years ago

with Ruby

The language used in #578 is called Liquid -- have a look when you get a chance: https://shopify.github.io/liquid/ It's "fun" (though, I feel I have to use triple air quotes around """fun""").

zkamvar commented 3 years ago

(though, I feel I have to use triple air quotes around """fun""").

Let's be honest, we need triple air fancy quotes around ‘‘‘fun’’’

zkamvar commented 3 years ago

I think this can be closed as it was superseded by #578