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

[Link] Improve TypeScript integration with react-router #15412

Merged
merged 2 commits into from
Apr 21, 2019
Merged

[Link] Improve TypeScript integration with react-router #15412

merged 2 commits into from
Apr 21, 2019

Conversation

pachuka
Copy link
Contributor

@pachuka pachuka commented Apr 19, 2019

Closes #15105, closes #15299, based on 3rd suggestion in #15299 (comment)

Since TypographyProps is currently not using OverridableComponent yet, I omitted the 'Component' prop for now to avoid conflicting types.

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 19, 2019

No bundle size changes comparing 1bff0f7...cb3c1b9

Generated by 🚫 dangerJS against cb3c1b9

@pachuka pachuka changed the title v4 typings for Link component [Link] v4 typings Apr 19, 2019
@eps1lon
Copy link
Member

eps1lon commented Apr 19, 2019

Feel free to create a Link.spec.tsx. Would be nice to have some tests for integration with react-router-dom.

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.

Nice, an integration test would be great. Ideally, I would add a live demo in https://next.material-ui.com/style/links/#third-party-routing-library. We might already have one demo for the buttons, we can copy.

@oliviertassinari oliviertassinari changed the title [Link] v4 typings [Link] Improve TypeScript integration with react-router Apr 19, 2019
@oliviertassinari oliviertassinari added the component: link This is the name of the generic UI component, not the React module! label Apr 19, 2019
@oliviertassinari oliviertassinari self-assigned this Apr 20, 2019
@oliviertassinari
Copy link
Member

I have added a live demo for the link, like @eps1lon did for the button. We should be good to go.

@pachuka
Copy link
Contributor Author

pachuka commented Apr 20, 2019

@oliviertassinari - you beat me to it! Just a quick question, when I was looking at other places that react-router-dom is used in the docs such as docs/src/pages/demos/breadcrumbs/RouterBreadcrumbs.js the Router is wrapped in a <NoSsr> component with the following comment: // Use NoSsr to avoid SEO issues with the documentation website. Do we need to do that here as well?

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 20, 2019

@pachuka Good question. The NoSsr is more a hack than a good solution (Google Bot can still see it with its JavaScript rendering engine). I have solved the problem by using URLs that exist in the documentation. I want to avoid linking 404.

@oliviertassinari oliviertassinari merged commit ebd8faa into mui:next Apr 21, 2019
@pachuka pachuka deleted the bugfix/ts-link-component-prop-types branch April 22, 2019 13:42
@eps1lon eps1lon mentioned this pull request May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: link This is the name of the generic UI component, not the React module! typescript
Projects
None yet
4 participants