-
Notifications
You must be signed in to change notification settings - Fork 981
fix: margin-header expects markdown
#1752
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
Conversation
|
/deploy-preview |
📝 Preview Deployment🔍 Full site preview: https://deploy-preview-1752.quarto.org 🔄 Modified Documents |
cderv
left a comment
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.
Did you notice any issues with the feature or just documentation problem to you ?
I believe this was documenting using .html because it made more sense as the services to extract the subscription code would produce a .html file, and the content is supposed to be HTML only, with no markdown.
You're right that the documentation does not mention the raw html part,
but this is because this is handled internally in the code.
function expandMarkdownFilePath(source: string, path: string): string {
const absPath = isAbsolute(path) ? path : join(dirname(source), path);
if (safeExistsSync(absPath)) {
const fileContents = Deno.readTextFileSync(absPath);
// If we are reading raw HTML, provide raw block indicator
const ext = extname(absPath);
if (ext === ".html") {
return "```{=html}\n" + fileContents + "\n```";
} else {
return fileContents;
}
} else {
return path;
}
}This logic applies for all header / footer (margin, body, sidebar)
So passing a .html file directly to margin-header is supposed to work ok and content will be wrapped in raw html block.
So I don't think the documentation should be adapted here.
cderv
left a comment
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.
Did you notice any issues with the feature or just documentation problem to you ?
I believe this was documenting using .html because it made more sense as the services to extract the subscription code would produce a .html file, and the content is supposed to be HTML only, with no markdown.
You're right that the documentation does not mention the raw html part,
but this is because this is handled internally in the code.
function expandMarkdownFilePath(source: string, path: string): string {
const absPath = isAbsolute(path) ? path : join(dirname(source), path);
if (safeExistsSync(absPath)) {
const fileContents = Deno.readTextFileSync(absPath);
// If we are reading raw HTML, provide raw block indicator
const ext = extname(absPath);
if (ext === ".html") {
return "```{=html}\n" + fileContents + "\n```";
} else {
return fileContents;
}
} else {
return path;
}
}This logic applies for all header / footer (margin, body, sidebar)
So passing a .html file directly to margin-header is supposed to work ok and content will be wrapped in raw html block.
So I don't think the documentation should be adapted here.
|
Documentation issue which in some aspect is confusing with other part of Quarto such as listing. The HTML file exception is to me more of a workaround from Quarto than a feature. To me this exception should remain a fallback but the documentation should be consistent, that is "use markdown with raw code block for HTML" as it's advertised everywhere else. |
It does not have the same code path as the listing, and there is no EJS templating here, so there will be a difference in documentation. Is this backed by a Github discussion or issue that shows the confusion?
The way it is added and documented leads me to believe this is a feature by convenience for users. This margin header is to be extracted from a service that will write HTML. It seems convenient to save it as a
I understand this. If it was a new config and new documentation, this would make sense to me to change. However, personally, I don't see the added value in modifying existing documentation that users may have already read and probably use the Anyhow, I just shared the possible reason why this part on "Subscriptions" for Blog is documented with an |
No, but my point is that using raw HTML in markdown without raw block leads to issues (the worst being in EJS files such as listings). I don't mind going the other way around and state that these
Even if Quarto was not added the raw block it would "work" in most cases as it "works" to have plain HTML in the middle of your document (until it does not). |
|
@cderv I'm closing this PR as I now believe the changes should be made on the other pages. |
Update the documentation to reflect that the
margin-headershould reference a markdown file/text instead of an HTML file.