Create helper to build public widget options for Sorter #2154
Conversation
…t function to filter out rubric data from widget options for the Sorter widget
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (2e38193) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2154If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2154 |
|
Size Change: +179 B (+0.01%) Total Size: 1.48 MB
ℹ️ View Unchanged
|
| correct: ["$0.005$ kilograms", "$15$ grams", "$55$ grams"], | ||
| layout: "horizontal", | ||
| padding: true, |
There was a problem hiding this comment.
@benchristel It seems like correct is the source of the card information in this widget, since there are no other fields with that data. Maybe sorting here would work as well? It looks like the first thing the widget does to render is shuffle the correct options anyway:
render(): React.ReactNode {
const options = shuffle(
this.props.correct,
this.props.problemNum as number,
/* ensurePermuted */ true,
);
I wonder if changing the type (to not Readonly to allow for modifying) so that it has the same field as widget options but with a different type makes it so that it won't work as a subtype of widget options though 🤔
ex. correct: ReadonlyArray vs Array
This can be seen in the PR for Matcher
There was a problem hiding this comment.
+1 for sorting. Could we also use ReadonlyArray in the public options type? I think that should work.
There was a problem hiding this comment.
Ah, you're right! Looks like it does. Probably because we do the slice and I just didn't update it. Will fix that, and then there's no issue. Though I was planning on changing the "correct" key to "cards" in the public type; would that be an issue for backwards compatibility between the types?
There was a problem hiding this comment.
If we change correct to cards, then the widget will have to be updated to use the correct prop when the answer has been revealed and the cards prop when it hasn't been. So it would complicate things.
This might be a good excuse to test out the migration facilities in the new perseus JSON parser. That would let us rename correct to cards on the PerseusSorterWidgetOptions type while remaining backwards-compatible with existing data. However, we can't do that until we have confirmed that the parsers are actually working in webapp, and it might be a sprint or two before that's done.
I would also support just adding a comment to the correct field to explain the situation.
There was a problem hiding this comment.
This might be a good excuse to test out the migration facilities in the new perseus JSON parser.
I'm fine if we decide to hold on this specific PR to wait until the parser is in place. Another (short-term) option is to bump the widget version and add a propUpgrades which is being renamed to widgetOptionsUpgrades. Only a couple of widgets do this right now and it will likely be deprecated in favor of the parser (I think???)
What would need to happen:
- We bump the widget version (
versioninWidgetExports) to a new major version - We update the editor to have both
correct(correct order) andcards(some arbitrary order) when it saves new Sorter widgets - We add a
propUpgradesthat takes old versions of the widget and transforms them into the correct/cards format - We update the widget to handle the correct/cards format
Obviously this will only work while we're sending the full JSON to the FE, but it will at least set us up for future changes. Once we only send the split JSON to the FE, we'll need to either:
- Run a backfill to upgrade all old Sorters
- Parse/upgrade widgets before sending them to the FE (maybe just the first time we find an old version and then we can rewrite the widget in GCS)
This also makes me realize we need to be sure to upgrade JSON before splitting it.
I'm thankful you all found this - it's exactly the kind of thing we need to know about! Since it has wider implications I'll CC @jeremywiebe
There was a problem hiding this comment.
I think this is something that doesn't need to be figured out now. We can shuffle the data for the public widget options, but keep it named correct (and document it clearly as Ben suggests).
This means that in different types, correct will mean different things (in the public widget options, it's the initial state, in the rubric (full) type, it's the correct answer).
This isn't great, but I think results in the least churn/uncertainty right now (no logic changes needed).
I've noted this issue in the Content Issues for Backfill doc.
There was a problem hiding this comment.
Sounds good! I'll leave it as correct with a note, and we can come back to it later. Thanks so much, everyone!
| correct: ["$0.005$ kilograms", "$15$ grams", "$55$ grams"], | ||
| layout: "horizontal", | ||
| padding: true, |
There was a problem hiding this comment.
I think this is something that doesn't need to be figured out now. We can shuffle the data for the public widget options, but keep it named correct (and document it clearly as Ben suggests).
This means that in different types, correct will mean different things (in the public widget options, it's the initial state, in the rubric (full) type, it's the correct answer).
This isn't great, but I think results in the least churn/uncertainty right now (no logic changes needed).
I've noted this issue in the Content Issues for Backfill doc.
…rted Sorted to remove correct order information
Summary:
Adds a function that takes Sorter's full widget options and filters out answer data. It also adds this function to Sorter's widget export and adds a test confirming the function does what is expected.
Issue: LEMS-2765
Test plan: