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.

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

[docs-infra] Allow to persist icons in ToC #37731

Merged
merged 6 commits into from
Jun 30, 2023

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented Jun 26, 2023

In MUI X, we want to show pricing plan icons in our ToC - see mui/mui-x#9490

Preview: https://deploy-preview-37731--material-ui.netlify.app/experiments/docs/headers

@cherniavskii cherniavskii added the scope: docs-infra Specific to the docs-infra product label Jun 26, 2023
@mui-bot
Copy link

mui-bot commented Jun 26, 2023

Netlify deploy preview

https://deploy-preview-37731--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against ef95c92

Comment on lines 242 to 246
.replace(
/([\uE000-\uF8FF]|\uD83C[\uDC00-\uDFFF]|\uD83D[\uDC00-\uDFFF]|[\u2011-\u26FF]|\uD83E[\uDD10-\uDDFF])\uFE0F?/g,
'',
) // remove emojis
.replace(/<\/?[^>]+(>|$)/g, '') // remove HTML
Copy link
Member

Choose a reason for hiding this comment

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

For the history, we added this logic for the support page, e.g. https://mui.com/material-ui/getting-started/support/#blog. I'm not aware of any other places where we do this in the docs.

If we feel that the icons are wrong in the H2, we could also fix it by removing them:

Suggested change
.replace(
/([\uE000-\uF8FF]|\uD83C[\uDC00-\uDFFF]|\uD83D[\uDC00-\uDFFF]|[\u2011-\u26FF]|\uD83E[\uDD10-\uDDFF])\uFE0F?/g,
'',
) // remove emojis
.replace(/<\/?[^>]+(>|$)/g, '') // remove HTML

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding emojis, I don't have a strong opinion on this - I'm fine with showing them in ToC or removing them from H2 completely.

Regarding HTML, here is an example H2 from MUI X docs:

## Lazy loading [<span class="plan-pro"></span>](/x/introduction/licensing/#pro-plan)

Which is then converted to HTML:

Lazy loading <a href="/x/introduction/licensing/#pro-plan"><span class="plan-pro"></span></a>

If we keep the HTML above, React throws an error because of the nested <a> tag:

Warning: Prop `dangerouslySetInnerHTML` did not match. Server: "Infinite loading " Client: "Infinite loading <a href=\"/x/introduction/licensing/#pro-plan\"><span class=\"plan-pro\"></span></a>

This is why I ended up keeping the HTML removal logic and adding the icon later if it's included in iconsToPersistInToC. I'm open to a more elegant solution if you have suggestions :)

Copy link
Member

Choose a reason for hiding this comment

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

React throws an error because of the nested tag:

Ah yes, the mismatch seems to come from the nested <a>, not good, this would fix it:

diff --git a/packages/markdown/parseMarkdown.js b/packages/markdown/parseMarkdown.js
index 3d8f9ddafc..3be9e3d23b 100644
--- a/packages/markdown/parseMarkdown.js
+++ b/packages/markdown/parseMarkdown.js
@@ -238,13 +238,8 @@ function createRender(context) {
         return `<h${level}>${headingHtml}</h${level}>`;
       }

-      const headingText = headingHtml
-        .replace(
-          /([\uE000-\uF8FF]|\uD83C[\uDC00-\uDFFF]|\uD83D[\uDC00-\uDFFF]|[\u2011-\u26FF]|\uD83E[\uDD10-\uDDFF])\uFE0F?/g,
-          '',
-        ) // remove emojis
-        .replace(/<\/?[^>]+(>|$)/g, '') // remove HTML
-        .trim();
+      // Remove links to avoid nested links in the TOCs
+      const headingText = headingHtml.replace(/<a\b[^>]*>/i, '').replace(/<\/a>/i, '');

       // Standardizes the hash from the default location (en) to different locations
       // Need english.md file parsed first

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, this looks better! 🎉

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we keep <code> tags in ToC?

master This PR

IMO, it looks slightly better with the <code> tags, especially on this page: https://deploy-preview-37731--material-ui.netlify.app/material-ui/experimental-api/css-theme-variables/usage/

@cherniavskii cherniavskii marked this pull request as ready for review June 27, 2023 09:48
@cherniavskii cherniavskii merged commit e6f223a into mui:master Jun 30, 2023
21 checks passed
@cherniavskii cherniavskii deleted the allow-to-persist-icons-in-ToC branch June 30, 2023 09:14
@oliviertassinari
Copy link
Member

Fair enough 👍

@oliviertassinari oliviertassinari added the enhancement This is not a bug, nor a new feature label Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is not a bug, nor a new feature scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants