-
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
Removing all instances of useV2Keypad #794
Conversation
Size Change: -120 B (0%) Total Size: 831 kB
ℹ️ View Unchanged
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #794 +/- ##
==========================================
+ Coverage 61.33% 62.65% +1.31%
==========================================
Files 405 407 +2
Lines 99429 99434 +5
Branches 6065 8559 +2494
==========================================
+ Hits 60989 62300 +1311
+ Misses 38440 37134 -1306
... and 31 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
Edited: I had a question about removing keypad-legacy as it's no longer used, but we've determined it's better to remove that at a later date. The minor codecov complaint can be ignored. :) |
@@ -89,7 +89,7 @@ class ProvidedKeypad extends React.Component<Props> implements KeypadAPI { | |||
this.props.onAnalyticsEvent({ | |||
type: "math-input:keypad-opened", | |||
payload: { | |||
virtualKeypadVersion: "MATH_INPUT_KEYPAD_V1", | |||
virtualKeypadVersion: "MATH_INPUT_KEYPAD_V2", |
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 think we should leave this unchanged until we actually remove the v1 keypad. This file is part of the v1 keypad so it feels really weird to be keeping this analytics event but firing the wrong virtualKeypadVersion
in it.
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.
@jeremywiebe @SonicScrewdriver v1 keypad has been removed as of this PR. Can we land this PR?
e8c3957
to
f34a125
Compare
f34a125
to
dda3cd2
Compare
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (dda3cd2) and published it to npm. You Example: yarn add @khanacademy/perseus@PR794 |
Summary:
Now that we've fully moved towards our v2keypad everywhere in webapp, we can remove the useV2Keypad from the keypad apiOptions. I also removed some errant keypad types that are no longer used: MATH_INPUT_KEYPAD_V1 and PERSEUS_MATH_INPUT. We'll also be able to clean this code up even further with this future ticket.
Issue: LC-1088
Test plan:
manual+automatic testing