Simplify InputWithExamples #1363
Conversation
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (4bcbdf3) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1363If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1363 |
|
Size Change: +91 B (+0.01%) Total Size: 845 kB
ℹ️ View Unchanged
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1363 +/- ##
==========================================
+ Coverage 69.76% 70.84% +1.07%
==========================================
Files 489 492 +3
Lines 103371 103274 -97
Branches 7452 11116 +3664
==========================================
+ Hits 72121 73167 +1046
+ Misses 31072 30107 -965
+ Partials 178 0 -178
... and 142 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -19,7 +17,6 @@ import type {StyleType} from "@khanacademy/wonder-blocks-core"; | |||
| const {captureScratchpadTouchStart} = Util; | |||
|
|
|||
| type Props = { | |||
| type: "math" | "text" | "tex"; | |||
There was a problem hiding this comment.
The removal of this prop will cause type errors in webapp when Perseus is updated. We'll need to bump the major version for this.
Alternatively, we could just mark this prop as deprecated, e.g.
/**
* @deprecated type has no effect and will be removed in a future release of Perseus
*/and release this change under a minor version bump. Then we could remove the prop in a future major release. I'd rather just bump the major version, though.
There was a problem hiding this comment.
I debated major vs minor (and even considered patch); I decided on minor since it's a change to a component and not a widget. The component is not exported, so I don't believe the external API of Perseus is changing - we're just changing how widgets consume an internal component.
I already started the upgrade process in Webapp to see if it breaks anything; if it throws errors I'll bump to major. (Right now it has issues with a flaky test, but otherwise looks good.)
There was a problem hiding this comment.
Ah cool! Yes, you are totally right about it not being exported. I would actually lean toward a patch release then, since I don't think this changes any observable behavior? I.e. we're not adding a feature, just deleting dead code.
There was a problem hiding this comment.
This is so weird! I also confirmed that InputWithExamples is not being used anywhere with a different type than "text", so I agree with you that this is the right way to go 👍🏽
It's odd to me that NumericInput doesn't use the type="math" version of this. And also that it uses InputWithExamples at all if it's not utilizing that?
|
@nishasy My guess is it was code predating the |
Summary:
While working on LEMS-2081 I started to get the hunch that InputWithExamples didn't use MathInput or MathOutput anymore.
The only uses I could find of InputWithExamples was InputNumber and NumericInput; both had hardcoded values for
type="text".I checked in
webappandmobiletoo. Think we're safe to clean this up if tests are passing.