-
Notifications
You must be signed in to change notification settings - Fork 350
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
Create a function to get the public options for LabelImage #2171
Conversation
…ic options for the LabelImage widget
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (3f9f0b7) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2171 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2171 |
Size Change: +108 B (+0.01%) Total Size: 1.48 MB
ℹ️ View Unchanged
|
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! Added some comments/questions, but nothing blocking. Looks like there are some prettier formatting issues in the label-image.tsx
file though
hideChoicesFromInstructions: boolean; | ||
multipleAnswers: boolean; | ||
static: boolean; |
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.
Curious why these ones are listed as boolean while the others reference the original widget options. Wouldn't it be good to reference the original type for all of the fields that are unchanged?
const {answers: _, ...publicData} = marker; | ||
return publicData; |
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.
So here, instead of building out the wanted object, you're using destructuring to pull out answers, is that correct? What does using the underscore do? Is that just to help signify that we aren't interested in the answers here? Looks like it's needed to satisfy ESLint.
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.
What does using the underscore do? Is that just to help signify that we aren't interested in the answers here?
Correct!
@@ -0,0 +1,5 @@ | |||
--- | |||
"@khanacademy/perseus-core": minor |
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.
Should there be a line here for perseus
since there are changes there as well?
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.
Good catch! I guess this is one reason not to generate the changeset until the PR is reviewed 🤔
Issue: LEMS-2769
Test plan:
yarn test