feat(curriculum): integrate MDN Curriculum #10433
Conversation
github-actions
bot
added
python
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
build/curriculum.ts
Outdated
| // @ts-ignore | ||
| import { renderHTML } from "../ssr/dist/main.js"; | ||
|
|
||
| export const allFiles = memoize(async () => { |
There was a problem hiding this comment.
Would indeed be helpful to merge #10567 first.
| property="item" | ||
| typeof="WebPage" |
There was a problem hiding this comment.
Are property and typeof here leftovers from the PreloadingDocumentLink component?
There was a problem hiding this comment.
Not straight forward and didn't want to break things.
There was a problem hiding this comment.
Looks like property and typeof refer to https://schema.org/BreadcrumbList, but is the information actually used by any tool?
| .curriculum-content-container.with-sidebar, | ||
| .curriculum-content-container { |
There was a problem hiding this comment.
Again, selectors feel a bit redundant here
There was a problem hiding this comment.
CSS specificity. changed it to .curriculum-content-container.curriculum-overview to narrow it down.
| ? `${doc.title} | ${CURRICULUM_TITLE}` | ||
| : CURRICULUM_TITLE; |
There was a problem hiding this comment.
| ? `${doc.title} | ${CURRICULUM_TITLE}` | |
| : CURRICULUM_TITLE; | |
| ? `${doc.title} | ${TITLE_SUFFIX}` | |
| : TITLE_SUFFIX; |
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| import { renderHTML } from "../ssr/dist/main.js"; | ||
| import { CheerioAPI } from "cheerio"; |
| const index = await buildCurriculumIndex(({ url, slug, title }) => { | ||
| return { url, slug, title }; | ||
| }); |
There was a problem hiding this comment.
FYI
| const index = await buildCurriculumIndex(({ url, slug, title }) => { | |
| return { url, slug, title }; | |
| }); | |
| const index = await buildCurriculumIndex(({ url, slug, title }) => ({ url, slug, title })); |
build/curriculum.ts
Outdated
| prev && "children" in prev && delete prev.children; | ||
| next && "children" in next && delete next.children; |
There was a problem hiding this comment.
Why do we modify index (via prev/next) as a side-effect of this function? It looks like a separate concern.
build/curriculum.ts
Outdated
| cur: CurriculumIndexEntry[] | ||
| ): DocParent[] | null { | ||
| for (const entry of cur) { |
There was a problem hiding this comment.
| cur: CurriculumIndexEntry[] | |
| ): DocParent[] | null { | |
| for (const entry of cur) { | |
| entries: CurriculumIndexEntry[] | |
| ): DocParent[] | null { | |
| for (const entry of entries) { |
build/curriculum.ts
Outdated
| return prevNextFromIndex(i, index); | ||
| } | ||
|
|
||
| function breadPath( |
There was a problem hiding this comment.
Nit: Maybe rename to breadcrumbPath()?
libs/types/curriculum.ts
Outdated
| module = "module", | ||
| overview = "overview", | ||
| landing = "landing", | ||
| about = "about", |
There was a problem hiding this comment.
Nit: Would be better to uppercase the first letter to avoid mistaking these for object properties.
| module = "module", | |
| overview = "overview", | |
| landing = "landing", | |
| about = "about", | |
| Module = "module", | |
| Overview = "overview", | |
| Landing = "landing", | |
| About = "about", |
| export function fileToSlug(file: string) { | ||
| return file | ||
| .replace(`${CURRICULUM_ROOT}/`, "") | ||
| .replace(/(\d+-|\.md$|\/0?-?README)/g, ""); |
There was a problem hiding this comment.
Just wondering if \d+- should be removed anywhere in the name or just at the beginning of the filename?
build/curriculum.ts
Outdated
| const currentLvl = meta.slug.split("/").length; | ||
| const last = item.length ? item[item.length - 1] : null; | ||
| const entry = mapper(meta); | ||
| if (currentLvl > 2) { |
There was a problem hiding this comment.
| const currentLvl = meta.slug.split("/").length; | |
| const last = item.length ? item[item.length - 1] : null; | |
| const entry = mapper(meta); | |
| if (currentLvl > 2) { | |
| const currentLevel = meta.slug.split("/").length; | |
| const last = item.length ? item[item.length - 1] : null; | |
| const entry = mapper(meta); | |
| if (currentLevel > 2) { |
| options?: { | ||
| previousNext?: boolean; | ||
| forIndex?: boolean; | ||
| } |
There was a problem hiding this comment.
Maybe declare a default value to avoid having to null coalesce (options?) below?
| } | |
| } = { previousNext: false, forIndex: false } |
build/curriculum.ts
Outdated
| modules = (await buildCurriculumIndex())?.filter( | ||
| (x) => x.children?.length | ||
| ); | ||
| } else if (attributes.template === Template.overview) { | ||
| modules = (await buildCurriculumIndex())?.find( | ||
| (x) => x.slug === slug | ||
| )?.children; |
There was a problem hiding this comment.
Can't we build the index once to avoid duplicate work, even if buildIndex() inside is memoized?
| if doc["mdn_url"].startswith("/en-US/curriculum/"): | ||
| # Skip curriculum content for now. | ||
| return | ||
|
|
There was a problem hiding this comment.
Nit: Would have been better to move this behind line 224 (locale, slug = doc["mdn_url"].split("/docs/", 1)), so it works independent of en-US (DEFAULT_LOCALE).
| @@ -74,7 +74,7 @@ router.get( | |||
| proxyContent | |||
| ); | |||
| router.get( | |||
| "/[^/]+/blog($|/*)", | |||
| ["/[^/]+/blog($|/*)", "/[^/]+/curriculum($|/*)"], | |||
There was a problem hiding this comment.
Nit: Does this not work?
| ["/[^/]+/blog($|/*)", "/[^/]+/curriculum($|/*)"], | |
| "/[^/]+/(blog|curriculum)($|/*)", |
client/src/ui/base/_themes.scss
Outdated
| @@ -207,6 +207,71 @@ | |||
| --baseline-limited-check: #1e8e3e; | |||
| --baseline-limited-cross: #ea8600; | |||
|
|
|||
| --cur-bg-color: #fcefe2; | |||
There was a problem hiding this comment.
Nit: I'm not a big fan of the cur prefix, because it can be mistaken for "current". I think it would have been okay to use curriculum-.
client/src/ui/atoms/icon/index.scss
Outdated
| "star-filled", "star", "theme-os-default", "thumbs-down", "thumbs-up", "trash", | ||
| "trash-filled", "twitter", "unknown", "warning", "webview", "yes", | ||
| "yes-circle"; | ||
| "chrome", "critical", "cur-next", "cur-prev", "deno", "deprecated", "desktop", |
There was a problem hiding this comment.
Especially here, cur-prev sounds like current-previous.
| name="selected" | ||
| type="radio" | ||
| checked={i === tab} | ||
| onChange={() => setTab(i)} |
There was a problem hiding this comment.
@fiji-flo Should we not record the tab changes with Glean?
Summary
Integrate https://github.com/mdn/curriculum content into MDN.
Mainly adds two parts:
yarn build:curriculumwhich is similar to the regular content and blog build step.Fixes and changes along the way:
How did you test this change?
It's deployed to stage and QAed