-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
feat(v2): frontmatter-less: read first heading as title and use it in front-matter #4485
Conversation
[V1] Deploy preview failure Built without sensitive environment variables with commit c4c002b https://app.netlify.com/sites/docusaurus-1/deploys/6059031c94ccce0008ea2e52 |
Deploy preview for docusaurus-2 ready! Built without sensitive environment variables with commit c4c002b |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4485--docusaurus-2.netlify.app/ |
hey @armano2 , I'm a Docusaurus user. This feature sounds cool, will surely align to my usecase In your case, will it happen too that the |
yes, that's a default behavior, if no sidebar label is present title is used instead, in /api/CLI you can actually see that happening |
tests added to blog and docs |
e3f6cd5
to
be80201
Compare
--- | ||
|
||
# CLI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can rollback this md file change as proper tests are now added
--- | |
# CLI | |
title: CLI | |
--- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I think it's fine and we should dogfood this a bit too ;)
That looks nice, thanks for working on this! Will review this more deeply tomorrow. Was wondering, do you think we should handle pages like docs and blog posts and automatically add the h1 tag to templates? It looks like we have a non-uniform setup that complicates a bit the code as pages should not remove the markdown heading. I'd also like to have more tests for |
const heading = /^# (.*)[\n\r]?/gi.exec(result.content); | ||
if (heading) { | ||
if (result.data.title) { | ||
console.warn( | ||
`Duplicate title detected in \`${source || 'this'}\` file`, | ||
); | ||
} else { | ||
result.data.title = heading[1].trim(); | ||
if (removeTitleHeading) { | ||
result.content = result.content.replace(heading[0], ''); | ||
if (result.excerpt) { | ||
result.excerpt = result.excerpt.replace(heading[1], ''); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonder if we could extract this easily in a separate function and test this in isolation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could actually go few steps further in separate pr
index should jsut be an entry point with bunch of reexports and we could create multiple files with specific things.
this file is starting to become really long and unreadable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally agree :)
was like that before and want to refactor that for a long time already
this could be done but not in this PR, as we should not include to much changes at once
circleci is failing after merge with master, same issue is preset on master |
Thanks
yes we just removed CircleCI, will be fine now :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! that looks great
const heading = /^# (.*)[\n\r]?/gi.exec(result.content); | ||
if (heading) { | ||
if (result.data.title) { | ||
console.warn( | ||
`Duplicate title detected in \`${source || 'this'}\` file`, | ||
); | ||
} else { | ||
result.data.title = heading[1].trim(); | ||
if (removeTitleHeading) { | ||
result.content = result.content.replace(heading[0], ''); | ||
if (result.excerpt) { | ||
result.excerpt = result.excerpt.replace(heading[1], ''); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally agree :)
was like that before and want to refactor that for a long time already
i just realized that this behavior is undocumented, maybe we should add it to docs? |
oh yes that could be nice thanks :) We have a good place for that: https://docusaurus.io/docs/next/markdown-features/headings |
FYI I don't think it's a good idea to put the content title in the frontmatter object, it's a bit confusing because indeed it's not frontmatter. Just wanted to let you know in case you depend on Will revert this in #4590 |
I have reported a bug on how titles are handled after updating to latest stable version. I added an example of how this is broken, check the following link, intro.md https://codesandbox.io/s/vigorous-wood-d2cz7?file=/docs/intro.md |
I believe it is a bug fix, not a breaking change, as we should not have allowed you to write a h1 title in frontmatter + markdown, and it is something to avoid We didn't expect this would be annoying for anyone because it is a weird thing to do in the first place (or I'd like to know your motivations) |
Motivation
typescript-eslint/typescript-eslint#3147
fixes #4425
Have you read the Contributing Guidelines on pull requests?
yes
Test Plan
i'm unsure where to put unit tests for this as i can't find any other unit tests for utils#
or title field is usedcontent can/should be modified by hand to see if data is printed correctly.