Skip to content

[docs] Add Breadcrumbs TypeScript demos #15139

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

Merged
merged 21 commits into from
Apr 3, 2019
Merged

[docs] Add Breadcrumbs TypeScript demos #15139

merged 21 commits into from
Apr 3, 2019

Conversation

Adherentman
Copy link
Contributor

Add TypeScript demos for each of the existing breadcrumbs demos

This is my first PR here 😄, let me know if there's anything I missed 🤙 .

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 1, 2019

No bundle size changes comparing 240dc18...63eb35b

Generated by 🚫 dangerJS against 63eb35b

@Adherentman
Copy link
Contributor Author

I have done yarn lint , yarn lint:ci, yarn lint:fix, yarn prettier, docs:typescript:formatted, but I don't know why I can't pass ci to eslint?

@oliviertassinari oliviertassinari changed the title docs: Add breadcrumbs Typescript Demo [docs] Add breadcrumbs Typescript Demo Apr 1, 2019
@oliviertassinari oliviertassinari changed the title [docs] Add breadcrumbs Typescript Demo [docs] Add breadcrumbs TypeScript Demo Apr 1, 2019
@Dudrie
Copy link
Contributor

Dudrie commented Apr 1, 2019

Have you run yarn docs:typescript:check, so there aren't any typing errors?
And did you change the order of imports? That one occuered to me aswell - we must not change the order of imports (so no auto-import-formatting in vs code for example) because if one changes the order then the JS and TS demos won't be equivalent.

@Adherentman
Copy link
Contributor Author

@Dudrie yes, I get this error

~/project/material-ui(next) » yarn docs:typescript:check                                                                                                                                xuzihao@MacBook-Pro
yarn run v1.1.0
warning You are using Node "10.14.2" which is not supported and may encounter bugs or unexpected behavior. Yarn supports the following semver range: "^4.8.0 || ^5.7.0 || ^6.2.2 || ^8.0.0"
$ tslint -p docs/tsconfig.json

ERROR: /Users/xuzihao/project/material-ui/docs/src/pages/demos/snackbars/IntegrationNotistack.tsx:4:55 - TypeScript@next compile error:
Module '"../../../../../../../../../Users/xuzihao/project/material-ui/node_modules/notistack"' has no exported member 'withSnackbarProps'.

error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

But this is not a file I wrote.

Then I sorted the import order in the ts file the same as js. But still haven't tested_build - Are the compiled TypeScript demos equivalent to the JavaScript demos?

@Dudrie
Copy link
Contributor

Dudrie commented Apr 1, 2019

One thing I noticed is that you did two things in one PR:

  • Adding the ts demo.
  • Changing the implementation to use hooks.

As @oliviertassinari mentioned in his Hook demos issue, we should do one PR for the demo (so the tests check if the demos are equivalent) and one extra PR for the conversion into a hook.

The question is (and I would wait for an answer, before doing any further action): Can this PR be an exception to that statement or should you "undo" the conversion to hooks? 🤔

Maybe this prevents you from trying things the whole afternoon on why the test_build fails 🙈.

@Adherentman
Copy link
Contributor Author

@Dudrie Thanks, I will continue to try

@Dudrie
Copy link
Contributor

Dudrie commented Apr 1, 2019

@Dudrie yes, I get this error

~/project/material-ui(next) » yarn docs:typescript:check                                                                                                                                xuzihao@MacBook-Pro
yarn run v1.1.0
warning You are using Node "10.14.2" which is not supported and may encounter bugs or unexpected behavior. Yarn supports the following semver range: "^4.8.0 || ^5.7.0 || ^6.2.2 || ^8.0.0"
$ tslint -p docs/tsconfig.json

ERROR: /Users/xuzihao/project/material-ui/docs/src/pages/demos/snackbars/IntegrationNotistack.tsx:4:55 - TypeScript@next compile error:
Module '"../../../../../../../../../Users/xuzihao/project/material-ui/node_modules/notistack"' has no exported member 'withSnackbarProps'.

error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Try running just yarn again, so all packages are at the version they should be. notistack is exporting a member withSnackbarProps and comes with proper ts definitions, so there's something strange happening here.
If that does not work, try deleting the node_modules folder, install all packages again and then try again, so you test with a clean, new installation of all packages.

However, according to the output of the test_unit test, the test itself does not fail because of that error (in fact it does not even raise that one) 🙈

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation typescript labels Apr 1, 2019
@Adherentman
Copy link
Contributor Author

Adherentman commented Apr 2, 2019

@Dudrie @oliviertassinari

Thank you for your help, I solved the above problem independently.. 🤗

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.

There are 2 casts here that are not ideal. We can follow up with an improvement in a future PR though.

@eps1lon eps1lon mentioned this pull request Apr 3, 2019
@eps1lon eps1lon merged commit 638ca64 into mui:next Apr 3, 2019
@eps1lon eps1lon changed the title [docs] Add breadcrumbs TypeScript Demo [docs] Add Breadcrumbs TypeScript demos Apr 3, 2019
@eps1lon
Copy link
Member

eps1lon commented Apr 3, 2019

@Adherentman Much appreciated, thanks.

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 typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants