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

Remove empty title tags in generated tests #2166

Closed
lhw-1 opened this issue Feb 13, 2023 · 3 comments · Fixed by #2186
Closed

Remove empty title tags in generated tests #2166

lhw-1 opened this issue Feb 13, 2023 · 3 comments · Fixed by #2186
Assignees

Comments

@lhw-1
Copy link
Contributor

lhw-1 commented Feb 13, 2023

Please confirm that you have searched existing issues in the repo

Yes, I have searched the existing issues

Any related issues?

#2128

Tell us about your environment

Ubuntu 20.04

MarkBind version

4.0.2

Describe the bug and the steps to reproduce it

The issue was brought up during the code review in PR #2128 where functional tests generated for template files will have an empty title tag if it has not been defined in site.json, even if it was defined in the frontmatter of the markdown file. An example can be seen here.

Expected behavior

The title tag should not be generated in the first place if there is no title declared.

Anything else?

No response

@lhw-1
Copy link
Contributor Author

lhw-1 commented Feb 23, 2023

Update:

The initial issue was brought up due to empty title tags in some of the functional tests, and upon investigation, this was solely because the title property was not declared in either the site.json or the frontmatter of the markdown file for certain test cases.

In packages/core/src/Page/index.ts, we are currently asserting the title of a site to exist, but declare it as an empty string if title has not been declared in either site.json or frontmatter, leading to the empty title tag.

Instead of removing the title tag altogether when empty, it would be a better practice to simply declare a default title for sites, e.g. "Default", as is currently being done for the layout property (with the default value declared here). The issue (and the PR targetting this issue) will be modified to reflect this.

@lhw-1 lhw-1 changed the title Remove empty title tags in generated tests Replace empty title tags in generated tests with default value Feb 23, 2023
@tlylt
Copy link
Contributor

tlylt commented Feb 28, 2023

Hi @lhw-1, thank you for working on this in PR #2186. Sorry for the late comment but I am wondering if there's a need to replace the empty title.

As you mentioned in the details, the issue is initially on the generation of an empty "title" element when there is no "title" declared, or when the title is implicitly set as "". I agree with @ong6 that if we can confirm that empty title elements serve no other purposes and do not raise any errors when omitted, then removing it is fine.

Replace the empty title element, however, will require us to provide a good default such that this default is better than having no default. So here are some things that I feel we need to consider:

  • when there's no title or when title is empty, a rendered page in the browser will still have a browser default, which is to show the URL of the page as its title. While this is likely not ideal, this is probably reasonable and people might expect this behavior if they know any HTML.
  • note that this is different from setting a default title in our templates, in which the default value is reasonable and obvious to the user as the default is defined in the page files.
  • also, the comparison to the default value used in the layout property isn't exactly right. Without the default layout, I suspect the site will have very much a degraded frontend, in this case the default is quite central and improves the user experience by a lot.
  • lastly, if we are going to consider finding a good default that is better than showing the URL of the page, we can look at other static site generators for inspiration, e.g. Gatsby, Jeykell, Docusaurus, and MkDocs. Whether they have an alternative default title can tell us a lot about the opinion of the devs on this. In Gatsby, it doesn't seem to have a default title.

Feel free to do some research and let me know what you think about the above.

@tlylt tlylt added s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding and removed c.Bug 🐛 labels Feb 28, 2023
@lhw-1
Copy link
Contributor Author

lhw-1 commented Mar 11, 2023

I've looked into some of the other static site generators mentioned by @tlylt, and the approaches seem to be quite mixed, though after looking at all the alternatives, I think that showing the URL of the page (as is being done) should be fine without needing to replace the header.

Gatsby and Docusaurus, after some basic testing on my system, seems to simply defaults to the URL of the page if no title is given (both in their front matter and site configurations equivalent). Jekyll, on the other hand, seems to take the name of the file and will capitalize it as the page title is none is given, which does seem to result in unwanted consequences for some users, but I think this is also an alright approach, if a bit unintuitive compared to the default. And MKDocs has not fully figured out the right way forward either 😅

This issue opened for MKDocs outlines a good discussion of the different thoughts on the matter, but overall, their current method seems be to extract the page title from the file name, similar to Jekyll.

Replace the empty title element, however, will require us to provide a good default such that this default is better than having no default.

So after looking through alternative approaches, I fully agree on this point, and without a good reason to override this, it would probably be better to leave the default behavior as it currently is. I will change back the issue to simply removing any empty title tags if there are no further objections 👍

@tlylt tlylt removed the s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding label Mar 11, 2023
@lhw-1 lhw-1 changed the title Replace empty title tags in generated tests with default value Remove empty title tags in generated tests Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants