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.

Sign up for GitHub

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(markdown): add support for GFM noteblocks proposal #10168

Merged
merged 10 commits into from
May 2, 2024
Merged

feat(markdown): add support for GFM noteblocks proposal #10168

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b2f9a46
Add support for GFM noteblocks proposal
queengooborg Dec 11, 2023
329150a
Add support for caution, tip and important notecard types
queengooborg Dec 11, 2023
0572d82
Keep with the three existing noteblock types
queengooborg Dec 14, 2023
6c7a111
Merge branch 'main' into gfm-noteblocks
queengooborg Apr 4, 2024
4efce0e
Remove preparation for other three types
queengooborg Apr 4, 2024
ad55837
Don't add space for Chinese locales
queengooborg Apr 4, 2024
8c4beb6
Merge branch 'main' into gfm-noteblocks
caugner Apr 16, 2024
11850be
chore(m2h): add getNotecardType() types
caugner Apr 16, 2024
1f824fc
chore(m2h): add getL10nCardMap() return type
caugner Apr 16, 2024
63a8e14
Update JSDoc...?
queengooborg Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 77 additions & 23 deletions markdown/m2h/handlers/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@ import { asDefinitionList, isDefinitionList } from "./dl.js";
import { Handler, Handlers, State } from "mdast-util-to-hast";

/* A utilitary function which parses a JSON gettext file
to return a Map with each localized string and its matching ID */
function getL10nCardMap(locale = DEFAULT_LOCALE) {
to return a Map with each key and its corresponding localized string.
If the locale specified is not found, it will fallback to English */
function getL10nCardMap(locale = DEFAULT_LOCALE): Map<string, string> {
// Test if target localization file exists, if
// not, fallback on English
let localeFilePath = new URL(
Expand All @@ -27,35 +28,61 @@ function getL10nCardMap(locale = DEFAULT_LOCALE) {

Object.keys(listMsgObj).forEach((msgName) => {
l10nCardMap.set(
listMsgObj[msgName]["msgstr"][0],
listMsgObj[msgName]["msgid"]
listMsgObj[msgName]["msgid"],
listMsgObj[msgName]["msgstr"][0]
Comment on lines +31 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This allows for a much easier lookup using GFM notecard keywords. The logic is reversed when using MDN-flavored syntax, so it iterates through each entry to find the applicable type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to add (key, value) and (value, key) to the map to allow both directions, or is there a risk for a collision? In either case, let's add a comment above to give an example what msgid and msgstr[0] refer to to help.

Also, let's update the JSDoc of the method to clearly indicate what the returning map contains.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might make sense to have variables for both formats, but I feel that if we are planning to migrate to GFM syntax, then I think it makes more sense to just have the one format, as it will make removal of the old syntax parsing much quicker.

);
});
return l10nCardMap;
}

function getNotecardType(node, locale) {
interface NotecardType {
type: string;
isGFM: boolean;
magicKeyword: string;
}

function getNotecardType(node: any, locale: string): NotecardType | null {
const types = ["note", "warning", "callout"];
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we shouldn't support callout here, if it isn't part of the GFM noteblocks proposal (and therefore not rendered by GitHub):

[!CALLOUT]
Callout

Important

Crucial information necessary for users to succeed.

Warning

Critical content demanding immediate user attention due to potential risks.

@mdn/core-yari-content What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: if we remove support for callouts in the GFM syntax, we'd have to special-case it for the original syntax. If the decision is not to support callouts in GFM syntax, then we should overhaul all existing callouts too.


if (!node.children) {
return null;
}
const [child] = node.children;
if (!child || !child.children) {
if (!child?.children) {
return null;
}
const [grandChild] = child.children;
if (grandChild.type != "strong" || !grandChild.children) {
return null;
}

// E.g. in en-US magicKeyword === Note:
const magicKeyword = grandChild.children[0].value;
const l10nCardMap = getL10nCardMap(locale);
let type;
if (l10nCardMap.has(magicKeyword)) {
const msgId = l10nCardMap.get(magicKeyword);
type = msgId.split("_")[1];

// GFM proposed notecard syntax -- https://github.com/orgs/community/discussions/16925
if (grandChild.type === "text") {
const firstLine = grandChild.value.split("\n")[0];
const match = firstLine.match(
new RegExp(`\\[!(${types.map((t) => t.toUpperCase()).join("|")})\\]`)
);
if (match) {
const type = match[1].toLowerCase();
const magicKeyword = l10nCardMap.get(`card_${type}_label`);
return { type, isGFM: true, magicKeyword };
}
}

if (grandChild.type === "strong" && grandChild.children) {
// E.g. in en-US magicKeyword === Note:
const magicKeyword = grandChild.children[0].value;

for (const entry of l10nCardMap.entries()) {
if (entry[1] === magicKeyword) {
const type = entry[0].split("_")[1];
return types.includes(type)
? { type, isGFM: false, magicKeyword }
: null;
}
}
}
return type == "warning" || type == "note" || type == "callout" ? type : null;

return null;
}

export function buildLocalizedHandlers(locale: string): Handlers {
Expand Down Expand Up @@ -86,18 +113,45 @@ export function buildLocalizedHandlers(locale: string): Handlers {
blockquote(state: State, node: any): ReturnType<Handler> {
const type = getNotecardType(node, locale);
if (type) {
const isCallout = type == "callout";
if (isCallout) {
if (node.children[0].children.length <= 1) {
node.children.splice(0, 1);
} else {
node.children[0].children.splice(0, 1);
const isCallout = type.type == "callout";

if (type.isGFM) {
// Handle GFM proposed syntax
node.children[0].children[0].value =
node.children[0].children[0].value.replace(/\[!\w+\]\n/, "");

// If the type isn't a callout, add the magic keyword
if (!isCallout) {
node.children[0].children.unshift({
type: "strong",
children: [
{
type: "text",
value: type.magicKeyword,
},
],
});
node.children[0].children[1].value =
(["zh-CN", "zh-TW"].includes(locale) ? "" : " ") +
node.children[0].children[1].value;
}
caugner marked this conversation as resolved.
Show resolved Hide resolved
} else {
// Remove "Callout:" text
if (isCallout) {
if (node.children[0].children.length <= 1) {
node.children.splice(0, 1);
} else {
node.children[0].children.splice(0, 1);
}
}
}

return {
type: "element",
tagName: "div",
properties: { className: isCallout ? [type] : ["notecard", type] },
properties: {
className: isCallout ? [type.type] : ["notecard", type.type],
},
children: state.wrap(state.all(node), true),
};
}
Expand Down