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] Localize the table of contents #14548

Merged
merged 14 commits into from Feb 19, 2019
Merged

[docs] Localize the table of contents #14548

merged 14 commits into from Feb 19, 2019

Conversation

mbrookes
Copy link
Member

@mbrookes mbrookes commented Feb 16, 2019

  • I have followed (at least) the PR section of the contributing guide.

  • Display the localized TOC instead of English

  • Localize the TOC heading: "Contents" -> t('tableOfContents')

  • Render valid hashes for UTF8 characters rather than stripping them out

  • Refactor using hooks

@mbrookes mbrookes added the docs Improvements or additions to the documentation label Feb 16, 2019
@mbrookes mbrookes added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Feb 16, 2019
@mbrookes mbrookes removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Feb 16, 2019
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

I suppose this was discussed privately? Could you add at least a short summary what the problem was and how you solved this? I see some regex was changed but without code comment or PR description the reason/intention of this change will be lost over time.

@mbrookes
Copy link
Member Author

I suppose this was discussed privately?

It was mentioned in the other TOC PR discussion here.

Could you add at least a short summary what the problem was.

The clue is in the title :D

The current regex doesn't work for unicode, due to \W stripping non ascii characters, so instead we filter common non-word characters 'manually'. The replacement regex is list of symbols to filter. Nothing more complicated than that.

@eps1lon

This comment has been minimized.

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.

Wow, I'm shocked by the simplicity of the code. It would have been a nightmare without the hook API 😳 ! The way we can memoize and derivate values is gold.

clearTimeout(unsetClickedRef.current);
window.removeEventListener('hashchange', handleHashChange);
};
}, [false]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}, [false]);
}, []);

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 18, 2019

Something might be wrong with the new hash logic:
Barre d'applications avec champ de recherche -> #barre-d39applications-avec-champ-de-recherche 🤔

It would be great to add a test, it's not something we want to see regressions on (it might partially break existing links).

@mbrookes mbrookes changed the title [docs] Fix fragment id for i18n [docs] Localize the table of contents Feb 19, 2019
@mbrookes
Copy link
Member Author

Something might be wrong with the new hash logic

The problem also existed in the old hash logic. Thanks for fixing it.

I have also tweaked the logic slightly to better match the old output. The test was a great help in making sure the change didn't introduce a regression. 👍

@mbrookes
Copy link
Member Author

That's odd. The tests that codecov is complaining about are for a file unrelated to this PR (material-ui-utils/src/getDisplayName.js), which, incidentally, seems to be dead code?

@@ -129,6 +128,21 @@ function getItemsClient(items) {
return itemsClient;
}

function useThrottledOnScroll(callback, delay) {
Copy link
Member

Choose a reason for hiding this comment

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

It sounds like we might be able to get rid of react-event-listener :) https://bundlephobia.com/result?p=react-event-listener@0.6.6.

@oliviertassinari oliviertassinari merged commit 692ea92 into mui:next Feb 19, 2019
@oliviertassinari
Copy link
Member

The codecov issue is solved on next.

@oliviertassinari
Copy link
Member

@mbrookes Well done :)

@mbrookes mbrookes deleted the docs-toc-i18n branch February 19, 2019 19:40
@mbrookes mbrookes mentioned this pull request Feb 25, 2019
1 task
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.

None yet

3 participants