Create an API Options context + use it within children of EditorPage #3013
Conversation
… within children of EditorPage
| issues?: Issue[]; | ||
| }; | ||
|
|
||
| const IssuesPanel = ({apiOptions, issues = []}: IssuesPanelProps) => { |
There was a problem hiding this comment.
I think I really like that we won't need to drill this APIOptions all over. Using a hook (or for our old class components, an HOC) will really clean things up and focus where we use APIOptions.
| return [ | ||
| <div className="perseus-editor-row" key={i}> | ||
| <fieldset disabled={editingDisabled}> | ||
| <APIOptionsContext.Provider |
There was a problem hiding this comment.
Yeah, this is what I was thinking. Components that are exported from @khanacademy/perseus-editor accept APIOptions as a prop, but then set up a context to provide it down the component tree. 👍
I think this gives us safety through TypeScript prop types, but relieves us from prop-drilling everywhere.
| } | ||
|
|
||
| getApiOptions(): APIOptionsWithDefaults { | ||
| getDeviceBasedApiOptions(): APIOptionsWithDefaults { |
| </div> | ||
| <APIOptionsContext.Consumer> | ||
| {({apiOptions}) => { | ||
| this.contextApiOptions = apiOptions; |
There was a problem hiding this comment.
Putting this in a field feels a bit risky for me but I can't quite express why. I think ideally we'd pass the apiOptions down to render functions, or better yet, we'd extract sub-components from this big Editor so they can just use the APIOptionsContext when they need.
There was a problem hiding this comment.
I agree it feels weird. I'll look into some other approaches.
🗄️ Schema Change: No Changes ✅ |
github-actions
bot
added
item-splitting-change
schema-change
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +140 B (+0.03%) Total Size: 498 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (439a016) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3013If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3013If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3013 |
Summary:
APIOptions is passed around everywhere, even in components it's not directly used
but is just passed down to children who use it.
It would clean up the code to put APIOptions in a context.
In this PR:
I'll have a different PR to add the context on the renderer side.
Issue: none
Test plan:
pnpm jestStorybook
/?path=/story/editors-editorpage--demo/?path=/story/editors-articleeditor--demo