-
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
Roll v2 Keypad Onto Desktop Web #711
Conversation
🦋 Changeset detectedLatest commit: 5033535 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (70a0ee8) and published it to npm. You Example: yarn add @khanacademy/perseus@PR711 |
Size Change: +2.51 kB (0%) Total Size: 852 kB
ℹ️ 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.
Whew, this ended up being quite a big change. I'm approving as the code looks fine (I do have some non-blocking thoughts).
But for this PR I think we should definitely test it in webapp before landing the PR (using the snapshot). This affects content editing as well as the expression widget so we need to make sure we don't break those usages.
cbfd5a6
to
401e296
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #711 +/- ##
==========================================
+ Coverage 59.59% 60.92% +1.32%
==========================================
Files 480 482 +2
Lines 105193 105430 +237
Branches 6025 8710 +2685
==========================================
+ Hits 62688 64228 +1540
+ Misses 42505 41202 -1303
... and 44 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
## Summary: In #711 we added some protective parsing code for AnswerForm `key`s. However, we discovered ([Slack](https://khanacademy.slack.com/archives/C3SLM30AW/p1701199431143119)) that there are instances of data where the key is entirely missing. Reverting the `throw` for now to unblock our content authors. I'll continue working on this in a separate PR/deploy to fix this more correctly as Ned was onto something! Issue: LC-1502 ## Test plan: Run the tests. They should pass. Author: jeremywiebe Reviewers: nedredmond Required Reviewers: Approved By: nedredmond Checks: ✅ gerald, ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage, ⏭ Publish npm snapshot, ✅ Extract i18n strings (ubuntu-latest, 16.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 16.x), ✅ Cypress (ubuntu-latest, 16.x), ✅ Jest Coverage (ubuntu-latest, 16.x), ✅ Check builds for changes in size (ubuntu-latest, 16.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 16.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 16.x), ✅ gerald Pull Request URL: #827
Screen.Recording.2023-08-03.at.2.55.21.PM.mov
Updates Perseus / Math Input to use keypad.
Also changes how it's used:
buttonsVisible
prop states changed:focused
default behavior, toggle off to startalways
default behavior, toggle on to startnever
toggle button disabledbuttonSets
type was marked as deprecatedkeypadButtonSets
prop added that takes new typebuttonSets
maps tokeypadButtonSets
extraKeys
prop that takes an array ofKeys
.