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] Fix some markdown spec issue #18428

Merged
merged 4 commits into from
Nov 18, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Nov 18, 2019

While mdx1 is currently not used in our docs it helped detect broken pages (~30 currently, fixes will be deployed to crowdin).

  • escape < and > when it isn't used for tags
  • consider tags as code e.g. <head> -> `<head>` (otherwise crowdin produces broken markup: https://material-ui.com/zh/styles/api/#stylesprovider)
  • close <img> tags
  • don't use style="" (here: extract inline svg into images which also makes them more accessible, reduces markdown parsing!, enables lazy loading)

1 can be considered as a stricter subset of markdown for this purpose.

I'll work on automating detecting and fixing these issues in another PR.

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Nov 18, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Nov 18, 2019

No bundle size changes comparing 3772c19...ff6bdc4

Generated by 🚫 dangerJS against ff6bdc4

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

@eps1lon What do you mean by "broken page"? The API markdown pages will likely soon be gone (so we can ignore them). The other pages seem to display correctly?

consider tags as code e.g. -> <head> (otherwise crowdin produces broken markup: https://material-ui.com/zh/styles/api/#stylesprovider)

This one is definitely broken, it would be great to solve it 👍.

While mdx is currently not used in our docs

Do we want to migrate to mdx? What would we gain vs the time we spend on it? At least, the performance aspect of it didn't seem to make a significant difference.

@eps1lon
Copy link
Member Author

eps1lon commented Nov 18, 2019

Do we want to migrate to mdx?

I'm experimenting with it on the side though it's not part of this discussion.

At least, the performance aspect of it didn't seem to make a significant difference.

The linked PR has nothing to do with what mdx does. The performance difference is significant (500-1000ms main thread work gone) but there are other issues that need to be solved. But again this is not part of this discussion.

What do you mean by "broken page"?

markdown is malformed resulting in display of raw html or missing formatting etc.

The API markdown pages will likely soon be gone (so we can ignore them)

Sure. As I said automatic task is not part of this PR.

@oliviertassinari
Copy link
Member

markdown is malformed resulting in display of raw html or missing formatting etc.

I don't fully understand. The content seems correctly formated when looking at https://material-ui.com/. However, it's not when we look at it from the GitHub interface. Is this accurate?

@eps1lon
Copy link
Member Author

eps1lon commented Nov 18, 2019

We have probably more than 500 pages in markdown. The landing page isn't the only one displayed in markdown. I'm happy to discuss with you whether you consider some of the pages currently broken. But, again, these fixes will be applied via crowdin not PR.

However, it's not when we look at it from the GitHub interface.

I don't think I changed pages that are usually viewed in GitHub (e.g. readmes, changelogs, etc)

@oliviertassinari oliviertassinari changed the title [docs] Fix some mdx compat issues [docs] Fix markdown spec issue Nov 18, 2019
@oliviertassinari oliviertassinari changed the title [docs] Fix markdown spec issue [docs] Fix some markdown spec issue Nov 18, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 18, 2019

I really don't understand, what does "broken" mean in this context? (https://material-ui.com/zh/styles/api/#stylesprovider is visually broken but the other pages seems to render identically)

@eps1lon
Copy link
Member Author

eps1lon commented Nov 18, 2019

I really don't understand, what does broken mean in this context?

I don't have other words for "broken formatting", sorry. I guess if you don't consider https://material-ui.com/zh/styles/api/#stylesprovider "broken" then not understanding what broken means is understandable.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 18, 2019

@eps1lon Thank you! So if I understand it correctly, mdx has helped to identify one broken visual formatting page (https://material-ui.com/zh/styles/api/#stylesprovider). The other changes help to better fit into the markdown spec. They have no visual impact be are necessary to automate the issue detection 👍 .

@eps1lon
Copy link
Member Author

eps1lon commented Nov 18, 2019

I isolated some of the issues: b8a1bf6

The link will likely get broken in the future since I heavily rebase while working.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 18, 2019

@eps1lon This issue is interesting:

-`Checkbox`コンポーネントは` FormControlLabel<code>コンポーネントを説明のラベルとして使うことができます。</p>
+`Checkbox`コンポーネントは` FormControlLabel` コンポーネントを説明のラベルとして使うことができます。

In the past, I had to go into the Crowdin interface to fix the translations (translations with missing closing tags </0>, </1>, etc). Maybe related to small mistakes by the translators.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 18, 2019

Notice that if we sum all the usage of the translations, they account for 12% of the usage of the documentation, 88% is still in english.

@eps1lon
Copy link
Member Author

eps1lon commented Nov 18, 2019

Notice that if we sum all the usage of the translations, they account for 12% of the usage of the documentation, 88% is still in english.

So is the argument to follow crowdin and drop these translations completely because no translation is better than a partial (or incorrect) translation?

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 18, 2019

@eps1lon I wish we could have usage stats from reactjs.org. It could help know: if there is potential for the translations, for instance, if they have a 40% usage rate of the translated documentation, then, we know it's important and that we don't perform well enough.

Capture d’écran 2019-11-18 à 12 43 00

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants