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 96 forks source link

block new gem; add default title to reference.md #634

Open zkamvar opened 2 years ago

zkamvar commented 2 years ago

There was an issue where GitHub had adopted the jekyll-titles-from-headings gem in their builds so that pages without default titles would have the first heading adopted as the page title. This commit disables that gem so that unintentional titles do not appear.

This will fix https://github.com/carpentries/styles/issues/633

maxim-belkin commented 2 years ago

I am 👍 to add a default title to reference.md (perhaps also need to drop one from the corresponding file in _layouts) but I am not sure about the titles_from_headings - I feel like this is a reasonable behavior.

zkamvar commented 2 years ago

but I am not sure about the titles_from_headings - I feel like this is a reasonable behavior.

In the context of a blog, I would agree, but in the context of our lessons---which were developed without this gem in place---it has led to unexpected outcomes. IMO, it would be better to disable this for our lessons going forward as opposed to making lesson maintainers come up with an appropriate heading for their reference pages.

brownsarahm commented 2 years ago

I agree with @zkamvar about it being unexpected behavior and since it's also not explicit (it's a hidden default) it's hard for maintainers, many of whom are novices with respect to the build systems, so I think it's more inclusive and will lead to better experiences for our maintainers to remove this.

maxim-belkin commented 2 years ago

In the context of a blog, I would agree, but in the context of our lessons---which were developed without this gem in place---it has led to unexpected outcomes.

I guess it depends on how we look at this. If this plugin was disabled in the first place, we would've encountered a similarly-confusing issue -- empty page titles on the reference pages in all of our lessons -- and we could just as well argue that having some default auto-generated page title is better than an empty one (even though neither scenario is what we expect).

since it's also not explicit (it's a hidden default) it's hard for maintainers

I agree that implicit settings can sometimes make things more difficult, but in this specific case I believe this plugin/gem provides a safer behavior when compared to not having a page title at all (this is what Zhian discovered in https://github.com/carpentries/styles/issues/633#issuecomment-990523852).

Also, the change that actually fixes the problem is the one where we set page title in the reference.md file itself, not the change where we disable the plugin. This latter change would apply to new types of pages that The Carpentries will create in the future (if any) and where we forget to set the title (or set it in the wrong place).

I think the right solution to make things easier for new maintainers is to "document" all the default GitHub Pages settings in our _config.yaml by listing them (in a commented-out form) with explanations, so that all these "defaults" become more visible and, hence, more accessible.

zkamvar commented 2 years ago

I agree that implicit settings can sometimes make things more difficult, but in this specific case I believe this plugin/gem provides a safer behavior when compared to not having a page title at all (this is what Zhian discovered in #633 (comment)).

To clarify, the comment you reference above does not indicate that the page has no title element, it indicates that the page has no page.title variable (both examples were with the plugin enabled). A missing page.title variable is a situation that the lessons have been handling for a long time already:

For our lessons, if there is no page title, the title of the page defaults to the site title:

https://github.com/carpentries/styles/blob/6f490499686e871d56d2aacdbdae5797385f1238/_layouts/base.html#L33-L35

The first heading of the page follows the same logic:

https://github.com/carpentries/styles/blob/6f490499686e871d56d2aacdbdae5797385f1238/_includes/main_title.html#L8

It's not a case of having a title vs. not having a title. It's the case of github trying to solve a problem that the maintainers anticipated and solved years ago.

Also, the change that actually fixes the problem is the one where we set page title in the reference.md file itself, not the change where we disable the plugin. This latter change would apply to new types of pages that The Carpentries will create in the future (if any) and where we forget to set the title (or set it in the wrong place).

Why not both? Changing the boilerplate affects new contributions, but does not solve the issue for existing maintainers. Changing both fixes the issue for existing maintainers and gives the new maintainers a clearer path for customisation.

I think the right solution to make things easier for new maintainers is to "document" all the default GitHub Pages settings in our _config.yaml by listing them (in a commented-out form) with explanations, so that all these "defaults" become more visible and, hence, more accessible.

This is out of scope for this particular PR.

maxim-belkin commented 2 years ago

Also, the change that actually fixes the problem is the one where we set page title in the reference.md file itself, not the change where we disable the plugin. This latter change would apply to new types of pages that The Carpentries will create in the future (if any) and where we forget to set the title (or set it in the wrong place).

Why not both? Changing the boilerplate affects new contributions, but does not solve the issue for existing maintainers. Changing both fixes the issue for existing maintainers and gives the new maintainers a clearer path for customisation.

Because disabling the gem doesn't solve the problem nor gives the new maintainers a clearer path for customisation. The solution that fixes the problem is setting the title in the reference.md file itself (what you did in the boilerplate file).

If we look at the files in the _layouts directory, the only file that tries to set page title using Jekyll front matter approach is exactly reference.html (grep 'title:' _layouts/*) and that is the mistake -- apparently, it doesn't work and we can't set a page title this way. Other layouts use HTML approach (some by specifying layout: base).

In a local test I disabled the gem, made sure that the /reference.md doesn't set title: ... in its front matter (common case across most lessons at the moment), and kept title: "Reference: in _layouts/reference.html. The result is that title in the generated reference.html file is set to the value of site.title (no word "Reference") -- this is equally confusing to what Sarah observed. Essentially, disabling the gem results in page.title of pages that don't set title using HTML to an empty string. I don't see how this is any less confusing than what was reported.