Skip to content

Conversation

@lhw-1
Copy link
Contributor

@lhw-1 lhw-1 commented Feb 1, 2023

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
This PR resolves #242.

While the original issue was opened before PR #935 which overhauled the site created on markbind init to the template currently being used, the issue was not closed, even though the main issue of duplicate declaration of title is no longer a problem, as the title in index.md was removed in PR #935.

Modifications were made as per the suggestion made in the original issue to have the starter site use the typical technique (i.e. title is declared within the frontmatter tag in the .md file) instead of declaring it in site.json, as declaring the title in site.json will override the one declared within the frontmatter tag.

Once it's been checked that no documentation needs to be updated, the [WIP] will be removed from the title of the PR.

Anything you'd like to highlight/discuss:

Testing instructions:

Running markbind init to create a new site (after binding the MarkBind version to the one containing changes made in this PR) will create an index.md now containing title in its frontmatter tag, and a site.json that no longer declares the title.

Proposed commit message: (wrap lines at 72 characters)
Remove override of title property in the init site


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@tlylt
Copy link
Contributor

tlylt commented Feb 1, 2023

Thanks @lhw-1, though slightly unrelated, I was thinking if we could illustrate the various ways of using the title by making use of the rest of the pages in the template. For example, if you look at https://markbind-init-typical.netlify.app/contents/topic1.html
these pages do not have a proper title at all. What do you think?

@lhw-1
Copy link
Contributor Author

lhw-1 commented Feb 1, 2023

I was thinking if we could illustrate the various ways of using the title by making use of the rest of the pages in the template. For example, if you look at https://markbind-init-typical.netlify.app/contents/topic1.html these pages do not have a proper title at all. What do you think?

Once this PR gets merged, we would be switching to only declaring the title inside the frontmatter tag, so I agree - it would definitely be worth adding in titles to the topic pages. We would ensure consistency across the pages, and for new users, standardize the idea that having the title be in the frontmatter tag is the "normal" way to do things in MarkBind - not to mention that it would look nicer on a newly initialized MarkBind site.

I'll create a new PR afterwards to make necessary changes to the topic pages (or at least open a new issue on it). Thanks for the suggestion @tlylt!

@tlylt
Copy link
Contributor

tlylt commented Feb 1, 2023

We would ensure consistency across the pages, and for new users, standardize the idea that having the title be in the frontmatter tag is the "normal" way to do things in MarkBind - not to mention that it would look nicer on a newly initialized MarkBind site.

Just to clarify, I was thinking to illustrate all usage of the title attribute, just not all at once. so perhaps landing page will have title in frontmatter only, then topic1 will have title in site.json only, and topic2 will have both title in frontmatter and site.json.

Perhaps @damithc can share if that's too much for a new user? If so we can stick with one usage.

@damithc
Copy link
Contributor

damithc commented Feb 1, 2023

Just to clarify, I was thinking to illustrate all usage of the title attribute, just not all at once. so perhaps landing page will have title in frontmatter only, then topic1 will have title in site.json only, and topic2 will have both title in frontmatter and site.json.

Perhaps @damithc can share if that's too much for a new user? If so we can stick with one usage.

w.r.t. illustrating different usages in the init site, perhaps we should limit to illustrating variations that have visible differences in the page and are of interest to first time users? This is because the user is likely to use the generated site as the starting point for their own site. If we put too much stuff in it, the user will have to do a lot of clean up first. What do you guys think?

@lhw-1
Copy link
Contributor Author

lhw-1 commented Feb 1, 2023

Just to clarify, I was thinking to illustrate all usage of the title attribute, just not all at once. so perhaps landing page will have title in frontmatter only, then topic1 will have title in site.json only, and topic2 will have both title in frontmatter and site.json.

Ah, I see what you mean. I agree with @damithc that this might not be much use for new users as it won't have visible impacts, and it may also have adverse effects where it could be confusing for the new users if there are three documents each with different ways of declaring title. And of course, as @damithc mentioned, the user will have to clean this up (which may cause additional confusion if the user is new).

But I get the idea that perhaps this can also be useful for those who want to learn about the different methods of declaring title that MarkBind provides. An alternative may be to include it as a comment inside index.md, but if that's too much (since it would be the only comment in the file even though its importance is relatively low), perhaps we can at least talk about the declaration of title a bit more specifically in the relevant section of the user guide here, under Tweaking the Page Structure - Front-matter?

E.g., changing the contents of the warning box in the section from:

Note: Page properties that are defined in site.json for a particular page will override those defined in the front matter of the page.

To:

Note: Page properties that are defined in site.json for a particular page will override those defined in the front matter of the page. For example, setting the property title in site.json will always override the title declared in the front matter of the page.

And then perhaps we can also add a tip for advanced users telling them that this feature could allow them to serve the same page with multiple titles if the page is being served from a submodule, as originally mentioned in #242 as the reason for why there are two ways to declare title.

@tlylt
Copy link
Contributor

tlylt commented Feb 2, 2023

Note: Page properties that are defined in site.json for a particular page will override those defined in the front matter of the page. For example, setting the property title in site.json will always override the title declared in the front matter of the page.

Sure, add a link to the title property in site.json as well so that users can navigate there easily.

Ah, I see what you mean. I agree with @damithc that this might not be much use for new users as it won't have visible impacts, and it may also have adverse effects where it could be confusing for the new users if there are three documents each with different ways of declaring title.
An alternative may be to include it as a comment inside index.md, but if that's too much (since it would be the only comment in the file even though its importance is relatively low),

Let's not add the comment and keep to the frontmatter usage. Having this is better as it looks nicer with some default title and it's easy to change.

And then perhaps we can also add a tip for advanced users telling them that this feature could allow them to serve the same page with multiple titles if the page is being served from a submodule, as originally mentioned in #242 as the reason for why there are two ways to declare title.

This is good point, the rationale might be of interest to potential users, possibly add this in reuse-content.

@lhw-1
Copy link
Contributor Author

lhw-1 commented Feb 2, 2023

Thanks for the suggestions @tlylt, will update with those changes in mind!

@lhw-1
Copy link
Contributor Author

lhw-1 commented Feb 2, 2023

Documentation has been updated with the suggestions by @tlylt!

@lhw-1 lhw-1 changed the title [WIP] Remove override of title in the init site Remove override of title in the init site Feb 2, 2023
@tlylt tlylt requested a review from ong6 February 2, 2023 09:34
Copy link
Contributor

@ong6 ong6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think we can add an explicit example to this section of the userGuide to showcase how this might work. Or maybe point to some existing examples so that first-time users won't be too confused.

Something simple like an extract of the siteData as well as the frontmatter would suffice.

Also as a side note, this should be a Documentation update not a feature addition :>

<meta name="generator" content="MarkBind 4.0.2">
<meta name="viewport" content="width=device-width, initial-scale=1">
<title>Landing Page</title>
<title></title>
Copy link
Contributor

@ong6 ong6 Feb 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should look into removing this empty title tag as it holds no value, either in a separate PR (feel free to create an issue for this - if it doesn't exist) or in this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I was planning to open a new issue / PR for cleaning up the default / minimal site templates, so I will include this as part of the new issue 👍

@lhw-1
Copy link
Contributor Author

lhw-1 commented Feb 10, 2023

I personally think we can add an explicit example to this section of the userGuide to showcase how this might work. Or maybe point to some existing examples so that first-time users won't be too confused.

Something simple like an extract of the siteData as well as the frontmatter would suffice.

I've added an example in f398db4!

Also as a side note, this should be a Documentation update not a feature addition :>

PR has been updated accordingly 👍

Copy link
Contributor

@tlylt tlylt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lhw-1, LGTM.

@tlylt tlylt added this to the 4.0.3 milestone Feb 12, 2023
@tlylt tlylt merged commit f712d7e into MarkBind:master Feb 12, 2023
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 title of index.md created by init is overridden by site.json

4 participants