Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from

Conversation

zkamvar
Copy link
Contributor

@zkamvar zkamvar commented Dec 10, 2021

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 #633

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 carpentries#633
@maxim-belkin
Copy link
Contributor

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
Copy link
Contributor Author

zkamvar commented Dec 13, 2021

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
Copy link
Contributor

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
Copy link
Contributor

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 #633 (comment)).

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
Copy link
Contributor Author

zkamvar commented Dec 15, 2021

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:

<title>
{% if page.title %}{{ page.title }}{% endif %}{% if page.title and site.title %} &ndash; {% endif %}{% if site.title %}{{ site.title }}{% endif %}
</title>

The first heading of the page follows the same logic:

<h1 class="maintitle"><a href="{{ relative_root_path }}{% link index.md %}">{{ site.title }}</a>{% if page.title %}: {{ page.title }}{% endif %}</h1>

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Page titles are taken from the first heading if no title exists
3 participants