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

Merged
merged 14 commits into from Mar 15, 2023

Conversation

lhw-1
Copy link
Contributor

@lhw-1 lhw-1 commented Feb 23, 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:
Resolves #2166.

As pointed out in the issue thread, 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). This PR consists of a straightforward fix by implementing a default value for the title tag.

As per the discussion in the original issue, after comparing approaches taken by other static site generators, we decided to stick with the default behavior, and additionally address the original concern of removing any unnecessary empty title tags generated (as well as prevent any empty title tags from being generated in the future).

Anything you'd like to highlight/discuss:

The default value is currently set to "Default", but suggestions on a better title are needed.

(Setting the title as the name of the .md file could be a better idea)

Once the fix has been finalized, documentation (primarily the user guide) will be updated as well.

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Remove empty title tags in generated tests


Checklist: ☑️

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

@lhw-1 lhw-1 requested a review from a team February 23, 2023 17:39
Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

Hi @lhw-1, thank you for the work on this!
I agree with the point that you raised that maybe using the title of the file would be a better way to generate the default title. Default is urm.. not the best 😂

packages/core/src/Page/index.ts Outdated Show resolved Hide resolved
@lhw-1 lhw-1 changed the title Add default value for title tags Remove empty title tags in generated tests Mar 13, 2023
@lhw-1 lhw-1 requested a review from a team March 14, 2023 05:34
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.

LGTM

@tlylt tlylt added this to the v4.1.1 milestone Mar 15, 2023
@tlylt tlylt merged commit 320a0f2 into MarkBind:master Mar 15, 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.

Remove empty title tags in generated tests
3 participants