-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
client/modules/Preview: migrate to TypeScript, no-verify
#3820
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
base: develop
Are you sure you want to change the base?
Conversation
|
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. |
Release EnvironmentsThis Environment is provided by Release, learn more! 🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-d37c3856c3 |
|
Hi @raclim , this PR is based on https://github.com/processing/p5.js-web-editor/blob/develop/contributor_docs/pr05_2025_typescript_migration/index.md#configuration-summary (@NalinDalal noted in Discord). I wasn't sure if this was ready for work, or blocked, or if there was an issue. Thanks for checking it out when you can. |
|
Thanks for flagging this @ksen0 and for trying this out @NalinDalal! Before opening a pull request, we generally advise contributors to make sure that their work is associated with an open issue to avoid any potential extra work or miscommunication in expectations! You can read up more about it in our Preparing a Pull Request documentation or our overall Contributor Docs. For the TypeScript Migration, we've mostly been opening smaller issues/PRs to work incrementally, though we should probably be opening a general issue/sub-issues for better tracking. For now, would you be able to create an issue for this that briefly summarizes your proposed changes and attach it to this PR? In the meantime, I'll take a look over this PR and try to make a larger issue/update the docs for folks to reference for migration work! |
client/modules/Preview: migrate to TypeScript, no-verify
bit late, but i have created the corresponding issue for same, check it out here |
|
Thanks so much @NalinDalal for attaching the issue! Tagging @clairep94 as well, who led a lot of the initial momentum for the TypeScript Migration, but I'll try to follow up by later this week! |
raclim
left a comment
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 taking a look into this!
I think the migration for the files overall look good so far, but previewIndex.tsx doesn't seem to have the correct file content. Would you be able to update this and also merge in the new changes from the develop branch?
| @@ -0,0 +1,104 @@ | |||
| import { useMemo } from 'react'; | |||
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.
I don't think this file has the correct content—it seems to be a duplicate of client/modules/Preview/filesReducer.ts.
|
i don't know why, it shows |
Fixes #3828
Changes:
I have verified that this pull request:
npm run lint)npm run test)npm run typecheck)developbranch.Fixes #123