-
Notifications
You must be signed in to change notification settings - Fork 1
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
Refactor: MUI5, Typescript #31
Conversation
build(config): update config for TS adapt to current standards
refactor: upgrade to MUI5
test(refactor): port tests to Typescript Tests still need some improvements at this point.
When graasp/graasp-apps-query-client#71 will be merged, change back line 17 of package.json: "@graasp/apps-query-client": "github:graasp/graasp-apps-query-client#70-errors-mockApi"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💪 Thank you for this massive refactor !
I have left comments here and there, we can iterate on them, or add relevant issues and tackle them later.
I did not know tss-react
do you think you will use it over the new mui5 syntax ? This is interesting !
You may have to edit the cypress.yml workflow to use the new config file in order for your tests to pass in CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few changes still, but once they are done you can merge !
Co-authored-by: Basile Spaenlehauer <spaenleh@gmail.com>
Co-authored-by: Basile Spaenlehauer <spaenleh@gmail.com>
Co-authored-by: Basile Spaenlehauer <spaenleh@gmail.com>
Remove old and useless deps
add autoresize hook Fix types in multiple components Remove useless messages Remove custom member type def Adapt mock db Co-authored-by: spaenleh
@spaenleh, I think I addressed all your comments. I'll merge this afternoon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, just one last nitpick 😄
Amazing PR you did there ! Continue the good work 💪
🐸 🌟
| { | ||
deleteAppDataShouldThrow?: boolean | undefined; | ||
} | ||
| undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You dont need to specify undefined, the object is already marked as ?
which is the shorthand for undefined
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for paying attention to that!
Close #29
Close #30
Close #23
Close #32