-
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
Input Number Deprecation and Conversion into Numeric Input #1731
Conversation
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (3f29d8a) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1731 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1731 |
Size Change: 0 B Total Size: 872 kB ℹ️ View Unchanged
|
1336a33
to
c38dc8c
Compare
021b83b
to
a861d59
Compare
@@ -287,7 +296,7 @@ class EditorPage extends React.Component<Props, State> { | |||
<ItemEditor | |||
ref={this.itemEditor} | |||
itemId={this.props.itemId} | |||
question={this.props.question} | |||
question={convertDeprecatedWidgets(this.props.question)} | |||
answerArea={this.props.answerArea} |
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.
This could be a spicy move -- but it seems to work with editing, and I can't move the conversion logic any higher.
@@ -31,7 +31,7 @@ import type { | |||
PerseusWidgetsMap, | |||
} from "@khanacademy/perseus"; | |||
|
|||
// like [[snowman input-number 1]] | |||
// like [[snowman numeric-input 1]] | |||
const widgetPlaceholder = "[[\u2603 {id}]]"; |
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.
Not a critical change, but we might as well have our example use a widget name that will still exist after this work.
@@ -190,7 +190,8 @@ export type { | |||
Size, | |||
CollinearTuple, | |||
MathFormat, | |||
InputNumberWidget, // TODO(jeremy): remove? | |||
InputNumberWidget, // Used for usurpation of InputNumberWidget in perseus-editor | |||
NumericInputWidget, // Used for usurpation of InputNumberWidget in perseus-editor | |||
// Widget configuration types |
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.
Note to self: I should be able to fix this if I use the PerseusWidgetMap type -- then we don't need to export these.
@@ -115,6 +118,8 @@ export const replaceDeprecatedEditors = () => { | |||
replaceEditor("sequence", "deprecated-standin"); | |||
replaceEditor("simulator", "deprecated-standin"); | |||
replaceEditor("unit-input", "deprecated-standin"); | |||
|
|||
replaceEditor("input-number", "numeric-input"); | |||
}; |
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.
Note to self: Add comment here explaining the editor page logic so that someone can follow the trail, if this is where they start of their journey
_.reject(_.keys(widgets), function (name) { | ||
return widgets[name].hidden; | ||
return widgets[name].hidden || name === "input-number"; | ||
}), |
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.
We don't want to register the Input Number widget any more, as we want to remove it from the list of available widgets that Content Creators can use.
However, we ALSO want to keep the larger registration of it so that we can identify Input Number Widgets that need to be converted to Numeric Input widgets. That's why we're simply skipping input-number on this particular step.
@@ -243,7 +243,7 @@ export class NumericInput | |||
}); | |||
|
|||
return ( | |||
<div> | |||
<div className="Im-just-a-sweet-lil-numeric-input"> |
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.
Oops, need to remove this sweet lil' guy
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.
I'll actually be adding this back temporarily for aiding QA testing, and I will remove it prior to deploy.
@@ -15,7 +15,12 @@ import SectionControlButton from "./components/section-control-button"; | |||
import Editor from "./editor"; |
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.
All changes to this file will be reverted now that we're performing the conversion in Webapp.
// so that we can convert deprecated widgets to their modern equivalents when | ||
// content creators use the Editor Page to update content containing these widgets. | ||
// Currently, we're only converting input-number to numeric-input, | ||
// but we can add more conversions here in the future. |
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.
Absolutely LOVING the extensive explanations!!! Thank you!
if (newKey) { | ||
newContent = newContent.replace(oldKey, newKey); | ||
} | ||
} |
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.
NIT: This could be a reduce()
operation:
return Object.keys(renameMap).reduce((newContent, oldKey) => { ... }, json.content)
53eedb9
to
a398605
Compare
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
512ed24
to
5221038
Compare
: [this.state.json]; | ||
return sections.filter( | ||
(section): section is PerseusRenderer => section !== null, | ||
); |
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.
Nit: Perhaps this could use nullish coalescence?
_sections(): PerseusRenderer[] {
return Array.isArray(this.state.json)
? (this.state.json ?? [])
: [this.state.json];
};
@@ -350,7 +362,7 @@ export default class ArticleEditor extends React.Component<Props, State> { | |||
const newSection = | |||
i >= 0 | |||
? { | |||
widgets: sections[i].widgets, | |||
widgets: sections![i].widgets, |
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.
Shouldn't this be a question mark instead of an exclamation point?
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.
I'm fixing this one by getting rid if the silly local type that's faking the PerseusRenderer.
// Modernize the json content of a PerseusRenderer object | ||
// by converting deprecated widgets to their modern equivalents | ||
export const convertDeprecatedWidgets = ( | ||
json: PerseusRenderer, |
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.
Since json
can be null/undefined, shouldn't this technically be:
json: PerseusRenderer | null | undefined,
let convertedQuestionJson: PerseusRenderer = props.question; | ||
if (props.question) { | ||
convertedQuestionJson = convertDeprecatedWidgets(props.question); | ||
} |
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.
Since convertDeprecatedWidgets
takes null/undefined as a parameter option, couldn't this be simplified to:
const convertedQuestionJson = convertDeprecatedWidgets(props.question);
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.
Hmm open to feedback on this one!
The approach I've taken (in my upcoming updates) has been to ensure we're only ever calling convertQuestionJson if we already have the data. Otherwise we need a TON of null checks across our code. I've updated convertDeprecatedWidgets to no longer return null
"itemDataVersion", | ||
), | ||
json: json, | ||
question: json.question, |
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.
I'm not seeing where this.state.question
is used elsewhere. Can you point me to where it is used? I'm also wonder if it needs to be updated in changeJSON
.
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.
Whoops, thank you for catching this! It's a vestige of an old approach to fixing the json. Removed!
export const convertDeprecatedWidgets = ( | ||
json: PerseusRenderer, | ||
): PerseusRenderer => { | ||
// If there's no json, or no input-number widgets, then we can return the json as is |
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.
Nit: Maybe generalize the explanation to "If there's no json, or no widgets to convert, then we can return the json as is"
// Check the content string for any top-level input-number widgets | ||
if (inputNumberRegex.test(json.content)) { | ||
return true; | ||
} |
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.
This might be over-engineering, but to aid in possible future conversions, what do you think about setting things up this way:
const widgetRegExes = [/input-number \d+/]; // Add your widget expression here
if (widgetRegExes.some(regex => regex.test(json.content))) {
return true;
}
return true; | ||
} | ||
|
||
// If there's no input-numbers top-level, then check the widgets for any nested widgets |
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.
Nit: Generic comment here, too?
"If there's no deprecated widget in the top-level, then check for any nested widgets"
if (newKey) { | ||
return newContent.replace(oldKey, newKey); | ||
} | ||
return newContent; |
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.
Nit: Ternary operator?
return newKey ? newContent.replace(oldKey, newKey) : newContent;
…e files, and temporary addition of a new div to help QA be able to tell we've swapped to the new widget.
- Refactoring of a ton of tests across Perseus to remove Input Number - Pre-work for removing references to Input Number all together + stashed proof of concept that we're safe to go - Refactoring of the ArticleEditor so that we can keep the conversion "in house" - Addition of div around numeric input to support QA testing (will be removed after testing)
9851e9d
to
079fcfa
Compare
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.
Super well-organized!! Thanks for all the documentation, too.
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/[email protected] ### Minor Changes - [#1731](#1731) [`27126aa00`](27126aa) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Conversion of Input Number to Numeric Input ### Patch Changes - [#1846](#1846) [`8eb1ff5d1`](8eb1ff5) Thanks [@benchristel](https://github.com/benchristel)! - Internal: add widget parsers for ADR 773. - [#1839](#1839) [`150888870`](1508888) Thanks [@nishasy](https://github.com/nishasy)! - [Locked Figure Labels] Util function to generate spoken math + use it within Locked Point aria labels ## @khanacademy/[email protected] ### Minor Changes - [#1859](#1859) [`dcf1fbe35`](dcf1fbe) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Addition of a new alert for the content editors when Input numbers are converted to Numeric Inputs - [#1731](#1731) [`27126aa00`](27126aa) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Conversion of Input Number to Numeric Input ### Patch Changes - [#1839](#1839) [`150888870`](1508888) Thanks [@nishasy](https://github.com/nishasy)! - [Locked Figure Labels] Util function to generate spoken math + use it within Locked Point aria labels - Updated dependencies \[[`8eb1ff5d1`](8eb1ff5), [`150888870`](1508888), [`27126aa00`](27126aa)]: - @khanacademy/[email protected]
… logic (#1905) ## Summary: In order to unblock Perseus, we are reverting the following commits: - [x] [#1888](#1888) [d0e7a03](d0e7a03) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Fixing issue with Input Numbers that have a value of 0 - [x] [#1884](#1884) [b4cf444](b4cf444) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Ensuring UserInput and Rubric widget keys match for edge cases - [x] [#1879](#1879) [04d6e60](04d6e60) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Fixing conflicts that arose from scoring and widget conversion efforts - [x] [#1753](#1753) [c1ba55f](c1ba55f) Thanks [@handeyeco](https://github.com/handeyeco)! - Change ServerItemRenderer scoring APIs to externalize scoring - [x] [#1866](#1866) [94eba15](94eba15) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Fixing a regression and a bug in the Input Conversion Logic - [x] [#1859](#1859) [dcf1fbe](dcf1fbe) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Addition of a new alert for the content editors when Input numbers are converted to Numeric Inputs - [x] [#1731](#1731) [27126aa](27126aa) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Conversion of Input Number to Numeric Input ## Test plan: - manual testing Author: SonicScrewdriver Reviewers: catandthemachines, #perseus Required Reviewers: Approved By: catandthemachines Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ gerald, 🚫 Publish npm snapshot (ubuntu-latest, 20.x), 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), 🚫 Check for .changeset entries for all changed files (ubuntu-latest, 20.x), 🚫 Check builds for changes in size (ubuntu-latest, 20.x), 🚫 Cypress (ubuntu-latest, 20.x), ✅ gerald, ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1905
… logic (#1905) ## Summary: In order to unblock Perseus, we are reverting the following commits: - [x] [#1888](#1888) [d0e7a03](d0e7a03) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Fixing issue with Input Numbers that have a value of 0 - [x] [#1884](#1884) [b4cf444](b4cf444) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Ensuring UserInput and Rubric widget keys match for edge cases - [x] [#1879](#1879) [04d6e60](04d6e60) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Fixing conflicts that arose from scoring and widget conversion efforts - [x] [#1753](#1753) [c1ba55f](c1ba55f) Thanks [@handeyeco](https://github.com/handeyeco)! - Change ServerItemRenderer scoring APIs to externalize scoring - [x] [#1866](#1866) [94eba15](94eba15) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Fixing a regression and a bug in the Input Conversion Logic - [x] [#1859](#1859) [dcf1fbe](dcf1fbe) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Addition of a new alert for the content editors when Input numbers are converted to Numeric Inputs - [x] [#1731](#1731) [27126aa](27126aa) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Conversion of Input Number to Numeric Input ## Test plan: - manual testing Author: SonicScrewdriver Reviewers: catandthemachines, #perseus Required Reviewers: Approved By: catandthemachines Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ gerald, 🚫 Publish npm snapshot (ubuntu-latest, 20.x), 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), 🚫 Check for .changeset entries for all changed files (ubuntu-latest, 20.x), 🚫 Check builds for changes in size (ubuntu-latest, 20.x), 🚫 Cypress (ubuntu-latest, 20.x), ✅ gerald, ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1905
Summary:
This PR contains all the work necessary to deprecate our Input Number Widget, and convert it into the Numeric Input widget (sans actually deleting the Input Number Files, which will be handled in LEMS-2411).
The changes include:
NOTE:
There is a temporary wrapper div around the Numeric Input component in order to help QA be able to determine that the widget has been converted, as InputNumber/NumericInput are virtually identical otherwise. This will be removed prior to landing the ticket, and after QA testing has completed.This has been removed.Screenshot
Issue: LEMS-2408 & LEMS-2409
Test plan: