Skip to content

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
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?

return itemsClient;
}

function useThrottledOnScroll(callback, delay) {
Copy link
Member

@oliviertassinari oliviertassinari Feb 19, 2019

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/[email protected].

@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.

3 participants