Moving from sprint to backlog; please holler if there's a reason to bump up the priority.

I'm moving this into the sprint because:

• It's a necessary precursor to T309109, which blocks T304463. Both of these tasks should be prioritized for the Codex beta release, because they are critical to providing clear component documentation
• The latest VitePress updates included some nice new features that will improve the docs site
• I can work on this intermittently between higher priority sprint tasks

Change 805907 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[design/codex@main] [WIP] Update VitePress

https://gerrit.wikimedia.org/r/805907

@Catrope here are the issues I've run into with the update:

Syntax highlighting

First off, syntax highlighting just isn't working as well in some instances:

• Bash:
  
• A Vue code block that only contains a single component used in a template:
  

The latter is a significant issue for configurable demos. Not sure if this might be considered a bug in the syntax highlighter. FWIW, wrapping the component in <template> tags remedies the issue, but that's not an ideal solution.

Second, the syntax highlighter used by Vitepress has changed from Prism to Shiki. We will need to update the code in Wrapper.vue that previously used Prism to highlight the generated code sample used in configurable demos. I attempted to do this as it seemed straightforward, but kept getting a cryptic error when I tried to initialize the highlighter.

Test failures

Tests in the codex-docs package that use jest.requireActual() to make a copy of the vitepress package are failing:

Test suite failed to run

    Jest encountered an unexpected token

    Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax.

    Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration.

    By default "node_modules" folder is ignored by transformers.

    Here's what you can do:
     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it.
     • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/configuration
    For information about custom transformations, see:
    https://jestjs.io/docs/code-transformation

    Details:

    /Users/atomasevich/src/codex/node_modules/vitepress/dist/node/index.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){export { g as build, f as createMarkdownRenderer, e as createServer, d as defineConfig, a as defineConfigWithTheme, r as resolveConfig, b as resolveSiteData, c as resolveSiteDataByRoute, s as serve } from './serve-42a92e2f.js';
                                                                                      ^^^^^^

    SyntaxError: Unexpected token 'export'

      48 | // set for all test cases to avoid duplication
      49 | jest.mock( 'vitepress', () => {
    > 50 | 	const originalModule = jest.requireActual( 'vitepress' );
         | 	                            ^
      51 | 	return {
      52 | 		__esModule: true,
      53 | 		...originalModule,

      at Runtime.createScriptFromCode (../../node_modules/jest-runtime/build/index.js:1728:14)
      at src/components/wrapper/Wrapper.test.ts:50:30
      at Object.<anonymous> (src/composables/useCurrentComponentName.ts:1:1)

Another thing to note is that the code pane now comes with a built-in copy-code button. Should we remove ours in favor of this one?

I'd normally opt for baked-in functionality, but that component doesn't fulfill contrast requirements in my humble opinion.

@Volker_E Good point, and it's not very discoverable, either—you have to hover over the language tag to even see it. It'd be nice not to duplicate the copy code functionality, so I'll look into disabling the built-in one.

In T308488#8125620, @AnneT wrote:

@Volker_E Good point, and it's not very discoverable, either—you have to hover over the language tag to even see it. It'd be nice not to duplicate the copy code functionality, so I'll look into disabling the built-in one.

There doesn't seem to be a configuration option, but we could just remove the built-in copy buttons from the DOM?

See here for the original request, and here for the implementation of the composable. It looks for elements matching the selector '.vp-doc div[class*="language-"] > button.copy', so if we remove those, it should disable it.

In T308488#8125048, @AnneT wrote:

@Catrope here are the issues I've run into with the update:

Syntax highlighting

First off, syntax highlighting just isn't working as well in some instances:

• Bash:
  

You have to grant me permission to see that file. Thanks Phabricator :(

• A Vue code block that only contains a single component used in a template:
  

The latter is a significant issue for configurable demos. Not sure if this might be considered a bug in the syntax highlighter. FWIW, wrapping the component in <template> tags remedies the issue, but that's not an ideal solution.

Based on looking at the list of languages available in Shiki, I guessed that we need to use the vue-html language for these instead, and it looks like that works:

Second, the syntax highlighter used by Vitepress has changed from Prism to Shiki. We will need to update the code in Wrapper.vue that previously used Prism to highlight the generated code sample used in configurable demos. I attempted to do this as it seemed straightforward, but kept getting a cryptic error when I tried to initialize the highlighter.

As far as I can tell, Shiki is not well-suited to syntax highlighting within the browser the way Prism is. Instead, I've amended your patch to add a line that imports Prism's CSS (which was previously added by Vitepress). This means dynamic code samples are highlighted by Prism while everything else is highlighted by Shiki. That's a horrible hack, and I'll try some more things tomorrow to see if we can use Shiki in the browser too.

Test failures

Tests in the codex-docs package that use jest.requireActual() to make a copy of the vitepress package are failing:

Test suite failed to run

    Jest encountered an unexpected token

    Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax.

    Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration.

    By default "node_modules" folder is ignored by transformers.

    Here's what you can do:
     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it.
     • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/configuration
    For information about custom transformations, see:
    https://jestjs.io/docs/code-transformation

    Details:

    /Users/atomasevich/src/codex/node_modules/vitepress/dist/node/index.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){export { g as build, f as createMarkdownRenderer, e as createServer, d as defineConfig, a as defineConfigWithTheme, r as resolveConfig, b as resolveSiteData, c as resolveSiteDataByRoute, s as serve } from './serve-42a92e2f.js';
                                                                                      ^^^^^^

    SyntaxError: Unexpected token 'export'

      48 | // set for all test cases to avoid duplication
      49 | jest.mock( 'vitepress', () => {
    > 50 | 	const originalModule = jest.requireActual( 'vitepress' );
         | 	                            ^
      51 | 	return {
      52 | 		__esModule: true,
      53 | 		...originalModule,

      at Runtime.createScriptFromCode (../../node_modules/jest-runtime/build/index.js:1728:14)
      at src/components/wrapper/Wrapper.test.ts:50:30
      at Object.<anonymous> (src/composables/useCurrentComponentName.ts:1:1)

It looks like what's going on here is that jest.requireActual() is not smart enough to understand Vitepress's package.json, even though Node.JS's own require() function is. Basically, Vitepress specifies the following:

{
  "type": "module",
  "main": "dist/node/index.js",
  "types": "types/index.d.ts",
  "exports": {
    ".": {
      "types": "./types/index.d.ts",
      "import": "./dist/node/index.js",
      "require": "./dist/node-cjs/index.cjs"
    },
    "./dist/client/*": "./dist/client/*"
  },

Node correctly uses the "require" entry when trying to load through require(), but Jest seems to just look at the "main" entry, and then be surprised that that's an ESM module, even though both that fact ("type": "module") and the alternative CJS file for require were clearly advertised. I haven't found a solution to this yet; I'll try to see if this is fixed in a later version of Jest, or if we need to do something special in our Jest config, or if defining the mock a different way helps.

I tried upgrading to Jest 28 locally, and that fixed the issue I described above, but it led to a different issue: Vitepress contains code that detects whether it's running in a Node.js environment or a browser environment, the jsdom environment setting for Jest tricks that, and it ends up trying to pass a fake localhost URL to createRequire(), which fails. I was able to then circumvent that issue by unsetting global.document and global.window.document before jest.requireActual( 'vitepress' ), and restoring it after.

But then I realized that we don't really need to use the real Vitepress in our mocks in these tests, we just need the useRoute function. Nothing from Vitepress is used in the tested code. So I just deleted the originalModule variable, and that fixed everything.

We should still upgrade to Jest 28 some time, but that's now no longer tied to the Vitepress upgrade.

@Volker_E @Sarai-WMDE @bmartinezcalvo We have completed the VitePress update from an implementation standpoint, but I was hoping at least one of you could look over the updated site to check for any issues.

Note that this patch does not aim to make any design improvements, it only aims to maintain the status quo as much as possible, and to fix any errors/issues that arose due to new VitePress styles. The VitePress CSS has been updated quite a bit and is very specific in some cases, so we have had to override certain styles to maintain our theme and accessibility standards. Please base your review on whether there are any issues, regressions, or accessibility failures, not design improvements that you would like to see. Let's implement those in the future (you know I want to!!)

You can review the updated site here. Let me know if you have any questions!

@AnneT I've reviewed the updated site and I've found some things to fix:

1. The text size in the right menu is too small and with too big line height (13/28). I would use instead the same 14/20 used in the left menu.

2. The color with opacity used for the subsections in the left menu is not contrasted enough with the gray #F8F9FA background. I would use instead the same #213547 color of the title (or our color-base #202122 if possible) using a Regular text for these subsections (Bold for the section title and Regular for the subsections instead Medium as is used now)

3. Would it be possible to add a left line when the menu item is selected (as we have in the right menu)? It would help the user to understand which menu item is selected.

4. When the left menu is collapsed the text size for "Menu" and "Return to top" is too small (12px). Would it be possible to use 16px instead?

5. About the gray #F8F9FA background for the left menu, this gray color distracts from what is truly important on the page that is the component demo. Would it be possible to use white background and add a gray right divider to separate the menu from the demo as they have in the current Codex demo?

{F35467824}

Thanks for the update and the openness to feedback, @AnneT :D

I agree with Bárbara's points regarding font sizes and contrast. About point 3, I think that instead of adding another fixed line in the menu (it already has dividers), maybe we could apply a selected state like the one you used in your redesign proposal, Bárbara:

From my side,

1. I would increase the size of the site's title/logo (let's see when we can work on one!), and probably make it 20 (1.25em)/25 (1.25) – matching one of our potential heading styles in the Codex tokens.

2. The color of this main title on hover makes it too little contrasted, so I'd apply #404244 (@color-base--hover) to it instead of the current 0.6 opacity.

3. The collapsed version of the right-hand menu seems to not be working. You get an empty menu when you click the three-dot button:

4. In general, I would encourage us to use other system values when possible. We could replace the following vp styles (they were extracted using Chrome'S CSS overview, which is a beta feature – I'm unsure whether all these styles are actually being used):

Background colors:

#FFFFFFB3 by #fff (@background-color-base)
#F1F1F1 by #f8f9fa (@background-color-interactive-subtle)

Text colors:

#476582 by #2A4B8D (@color-progressive--active)
#3C3C3CB3 by #54595D (@color-subtle)
#213547 by #202122 (@color-base)

Border colors:

#3C3C3C1F and #3C3C3C4A by #c8ccd1 (@border-color-subtle)

Font weight:

Replace 500 by 600

Font sizes:

Apply 0.875em (14px) with a 1.4285714286 line-height (20px) instead of 12.8px (0.8em)/inherit to the 'Copy' buttons in the token visualization page.

In T308488#8165196, @Sarai-WMDE wrote:

1. I would increase the size of the site's title/logo (let's see when we can work on one!), and probably make it 20 (1.25em)/25 (1.25) – matching one of our potential heading styles in the Codex tokens.

@Sarai-WMDE @AnneT apart from increasing the size of the site's title, I would add the Wikimedia Foundation logo before "Codex" to indicate that this design system belongs to Wikimedia. I added it in my explorations to improve the demo site.

Thanks again, @bmartinezcalvo and @Sarai-WMDE! I've implemented all of the changes you suggested, with a few exceptions, listed below. You can see the changes here. I think it's a major improvement from before, so kudos to you both!

• For Barbara's second suggestion (sidebar nav colors), I updated some VitePress tokens to use our colors, and now nav headings will be @color-base while nav items will be @color-subtle. If the nav items need to be darker, I can change them to be @color-base
• For Barbara's third suggestion (active sidebar item), this (or the suggestion Sarai made) would be quite a bit of work to implement. We can totally do this down the road, but I'd prefer to wait because the patch is already really big. For now, I have bolded the active link so it's at least a little bit more visible.
• Sarai: I updated a bunch of the VitePress tokens to map them to ours, but I may not have hit all of them (their styles are pretty disorganized so it wasn't as simple as looking at their vars.css file and finding each token based on the values you provided). I also updated VitePress from alpha4 to alpha10 in the latest patch, so there are likely new styles in there. Do you mind taking a look at the site and letting me know what is still outstanding? We could even meet next week to go over this together so we can knock it out

Change 805907 merged by jenkins-bot:

[design/codex@main] docs: Update VitePress

https://gerrit.wikimedia.org/r/805907

@Catrope @Volker_E @egardner @DannyS712 FYI, the patch that was just merged introduced a change in the VitePress dev server, which now uses port 5173 to align with the new default dev port for Vite itself. So, when you run npm run doc:dev, you'll view the docs site at http://localhost:5173/ (unless you customize the port).

In T308488#8187291, @AnneT wrote:

Thanks again, @bmartinezcalvo and @Sarai-WMDE! I've implemented all of the changes you suggested, with a few exceptions, listed below. You can see the changes here. I think it's a major improvement from before, so kudos to you both!

@AnneT thank you, I've reviewed the last patch and the demo site looks really good with all these improvements! I think we should now update the Codex Improvements design prototype with these new updates and also test first in the prototype the design we want to implement. Once this prototype is updated we can talk again (maybe next week) so we can discuss about which of the improvements are easy to implement in this demo.

Some things I found in the last patch to fix:

1. It seems that there is a gray line on top of the logo in the header (you can view it if you make scroll in the top of the page)
   
2. I like the new solution to mark the selected menu item (I prefer the left line but if it's not possible this other solution with blue and bold text is good too). But I thing we need to improve a couple of things:
   • The hover state is not enough visible now so if the selected state is blue I would use color-progressive--hover for the hover state.
     
   • If we use Bold blue text for selected menu items then I think we should use the same solution for the right menu since now we have different selected menu items for the left and right menus.
     
3. Is it possible to delete this green color from the next pages selectors when hover? I would paint it in blue (as in the default state) and I would paint just the border in blue when hover.
   

Change 827560 had a related patch set uploaded (by Anne Tomasevich; author: Anne Tomasevich):

[design/codex@main] styles, docs: Improve more styles after the VitePress update

https://gerrit.wikimedia.org/r/827560

Thanks for catching these issues, @bmartinezcalvo! The fixes are staged here: https://827560--wikimedia-codex.netlify.app/

In T308488#8195928, @AnneT wrote:

Thanks for catching these issues, @bmartinezcalvo! The fixes are staged here: https://827560--wikimedia-codex.netlify.app/

@AnneT thank you! I list below (and in the task description) the things that still need to be fixed:

1. Use color-progressive--hover for unselected-hover menu items (currently hover uses black)
2. Selected menu items should not be hovered with color-progressive--hover

(Apply these corrections in both left and right menus)

@bmartinezcalvo Fixed and up on the demo site for the patch!

In T308488#8198783, @AnneT wrote:

@bmartinezcalvo Fixed and up on the demo site for the patch!

@AnneT perfect, all the items in the Design Review list have been fixed.

Great, thanks for your help with this!

Change 827560 merged by jenkins-bot:

[design/codex@main] styles, docs: Improve more styles after the VitePress update

https://gerrit.wikimedia.org/r/827560

Change 828083 had a related patch set uploaded (by Catrope; author: Catrope):

[mediawiki/core@master] Update Codex from v0.1.0-alpha.10 to v0.1.0

https://gerrit.wikimedia.org/r/828083

Change 828083 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from v0.1.0-alpha.10 to v0.1.1

https://gerrit.wikimedia.org/r/828083