Use new API context within widget editors #3024
Conversation
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: -95 B (-0.02%) Total Size: 498 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (8d71d4c) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3024If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3024If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3024 |
jeremywiebe
left a comment
jeremywiebe
left a comment
There was a problem hiding this comment.
I think I like this. I'd love to hear other folks' opinions but I think I'd be interested in proceeding with this.
| WidgetEditorState | ||
| > { | ||
| widget: React.RefObject<React.ElementRef<typeof Editor>>; | ||
| widget: React.RefObject<EditorType>; |
There was a problem hiding this comment.
We could just move the initialization up here (which I feel is pretty common in class components.
| widget: React.RefObject<EditorType>; | |
| widget = React.createRef<EditorType>; |
| showWidget: props.widgetIsOpen ?? true, | ||
| widgetInfo: _upgradeWidgetInfo(props), | ||
| }; | ||
| this.widget = React.createRef(); |
There was a problem hiding this comment.
☝️ Then you could remove this line...
| <DependenciesContext.Provider value={testDependenciesV2}> | ||
| <ImageEditor {...props} /> | ||
| <APIOptionsContext.Provider value={apiOptions}> |
There was a problem hiding this comment.
Not asking for a change now... but I suspect we might want a sort of test harness that wraps things in deps and api options... let's see how it goes. :)
| <DependenciesContext.Provider value={testDependenciesV2}> | ||
| <APIOptionsContext.Provider | ||
| value={{ | ||
| ...ApiOptions.defaults, | ||
| flags: getFeatureFlags({"image-widget-upgrade": false}), | ||
| }} | ||
| > | ||
| <ImageEditor onChange={onChangeMock} /> | ||
| </APIOptionsContext.Provider> | ||
| </DependenciesContext.Provider>, |
There was a problem hiding this comment.
You could allow ImageEditorWithDependencies to accept a Partial<APIOptions> and then blend that parameter with the defaults that are defined at the top of this file. Might be a touch cleaner... or not. 🤷♂️
| <DependenciesContext.Provider value={testDependenciesV2}> | |
| <APIOptionsContext.Provider | |
| value={{ | |
| ...ApiOptions.defaults, | |
| flags: getFeatureFlags({"image-widget-upgrade": false}), | |
| }} | |
| > | |
| <ImageEditor onChange={onChangeMock} /> | |
| </APIOptionsContext.Provider> | |
| </DependenciesContext.Provider>, | |
| <ImageEditorWithDependencies | |
| apiOptionOverrides={{ | |
| flags: getFeatureFlags({"image-widget-upgrade": false}), | |
| }} | |
| onChange={onChangeMock} | |
| /> |
| static propTypes = { | ||
| ...Changeable.propTypes, | ||
| togglePrompt: PropTypes.string, | ||
| definition: PropTypes.string, | ||
| apiOptions: PropTypes.any, | ||
| }; |
There was a problem hiding this comment.
Nice. Thanks for replacing these instances of PropTypes!
|
|
||
| // TODO(jangmi, CP-3288): Remove usage of `UNSAFE_componentWillMount` | ||
| UNSAFE_componentWillMount() { | ||
| this._editors = []; |
There was a problem hiding this comment.
Eww... can we just initialize this on line 18 and get rid of this UNSAFE_componentWillMount entirely?
| <Table {...(tableProps as PropsFor<typeof Table>)} /> | ||
| </div> | ||
| </div> | ||
| <APIOptionsContext.Consumer> |
There was a problem hiding this comment.
I didn't think of this till I got to the next file (packages/perseus-editor/src/editor.tsx), but you could probably make this PR somewhat less disruptive by wrapping these various widget editors using the withAPIOptions() HoC also. Then you wouldn't need to put the context consumer in all these render functions.
🤔
Summary:
todo
Issue: none
Test plan:
todo