-
Notifications
You must be signed in to change notification settings - Fork 481
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
Convert a few Blockly files to TypeScript #56314
Conversation
This PR is a continuation of #56171, which was mis-closed by the Git LFS migration (#55759). Previous Comments:Previous Reviews:
|
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.
Cool! I'm excited to see that these changes look fairly straight-forward! I don't think it had occurred to me that we could start to tackle this with small pieces like this. 🎉
if (mainWorkspaceFlyout) { | ||
mainWorkspaceFlyout.reflow(); | ||
} | ||
mainWorkspace?.getFlyout()?.reflow(); |
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.
Great find!
@@ -7,14 +7,15 @@ import { | |||
import moduleStyles from './modal-function-editor.module.scss'; | |||
import classNames from 'classnames'; | |||
import Button from '@cdo/apps/templates/Button'; | |||
import msg from '@cdo/locale'; | |||
import {commonI18n} from '@cdo/apps/types/locale'; |
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 including the link about this particular change!
Initial steps at adding some TypeScript to our Blockly code. I just converted a couple files here:
constants.js
->constants.ts
: the only change that was needed here was typing the parameters of two functions. We also were explicitly importing things from this file viaconstants.js
, so I updated those imports to just be fromconstants
.ModalFunctionEditor.jsx
->ModalFunctionEditor.tsx
: our only React component in the blockly folder. All that was required here was typing the state from redux and changing the locale import to the TypeScript version (see TypeScript locale completion #53623 for details on this)utils.js
->utils.ts
: this one was more interesting:reduce
in TypeScript utilizes generics, so you need to add a type to your call ofreduce
to specify what type it will return.findFlyout
, which relied on private fields in Blockly'sWorkspaceSvg
class (flyout
andtoolbox_.flyout_
. If we want to use TypeScript here, we could not reference a private field. I looked atWorkspaceSvg
to see if there was an alternative. Very conveniently,WorkspaceSvg.getFlyout()
was doing the exact same thing as this entire function (code). Therefore I replaced use of this function with blockly's function instead (it was only used in one place,eventHandlers
). We may not always get such a neat solution for our use of private fields, but this one ended up making our code shorter.Links
Testing story
I tested locally that Google Blockly labs (Dance Party and Sprite Lab on google blockly) still worked as expected, specifically around the modal function editor and browser resizing, which is what this touched.
Follow-up work
I am spending this week trying to move some of our Blockly code to TypeScript, so expect more PRs like this. I am trying to keep each PR relatively small and low risk.
PR Checklist: