-
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
[Interactive Graph Editor] Add/Edit/Delete Locked Function #1493
[Interactive Graph Editor] Add/Edit/Delete Locked Function #1493
Conversation
…r and line style options). Move common code for selecting line style to its own component (just like with the color selector).
… good way to load the parsed equation to save time, we still need to call 'parse()'. Adjust 'LockedFunction' jsx to only parse the equation when it is changed. Add equation field to graph settings.
… accordingly. Add directional axis and domain limits to settings panel.
…s a label for the equation. Update the logic that handles the entry of domain limits.
Update unit tests accordingly.
Size Change: +1.52 kB (+0.18%) Total Size: 855 kB
ℹ️ View Unchanged
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1493 +/- ##
==========================================
+ Coverage 70.30% 70.56% +0.25%
==========================================
Files 510 515 +5
Lines 105645 105967 +322
Branches 7690 11469 +3779
==========================================
+ Hits 74274 74775 +501
- Misses 31189 31192 +3
+ Partials 182 0 -182
... and 153 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 |
expect(onChangeProps).toHaveBeenCalledTimes(3); | ||
expect(onChangeProps).toHaveBeenNthCalledWith(1, {equation: "z"}); | ||
expect(onChangeProps).toHaveBeenNthCalledWith(2, {equation: "o"}); | ||
expect(onChangeProps).toHaveBeenNthCalledWith(3, {equation: "t"}); |
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 a little confused about this. How come it's not
expect(onChangeProps).toHaveBeenNthCalledWith(1, {equation: "z"});
expect(onChangeProps).toHaveBeenNthCalledWith(2, {equation: "zo"});
expect(onChangeProps).toHaveBeenNthCalledWith(3, {equation: "zot"});
?
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 added a note in the test explaining this. The reason is due to onChangeProps
being mocked, and not passing the typed letter to the equation field.
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.
Thank you!
expect(onChangeProps).toHaveBeenCalledWith({directionalAxis: "y"}); | ||
}); | ||
|
||
describe("Domain restrictions", () => { |
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.
Is this describe block supposed to be nested within the "settings interactions" describe block?
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.
Yes. Since domain restrictions have more tests, I wanted them grouped. Since these tests are still interaction tests, I kept them as a sub-block within "Settings interactions". Based upon your question, I've changed the title to be "Domain interactions" to be more consistent.
await userEvent.type(domainMaxField, "-2"); | ||
|
||
// Assert | ||
// expect(onChangeProps).toHaveBeenCalledTimes(2); |
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 suspect you didn't mean to leave this comment here
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.
Correct. Good catch. Thank you.
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 really appreciate the comprehensive testing!
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.
100% code coverage, or bust!
domain && domain[1] !== Infinity ? domain[1].toString() : "", | ||
]); | ||
|
||
useEffect(() => { |
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.
It sounds like this useEffect is here to handle the domain changing to/from infinity? Is that right?
Could you add a commenting explaining why this useEffect is necessary?
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.
Comment added.
)} | ||
/> | ||
</View> | ||
|
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: Add a comment here to stay consistent with the other comments.
{/* Directional axis settings */}
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.
Added.
const styles = StyleSheet.create({ | ||
accordionHeader: { | ||
textOverflow: "ellipsis", | ||
maxWidth: "calc(100% - 64px)", |
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.
Why was it necessary to use calc
here? I think padding
should work too, right?
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.
Explanation added.
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.
Thank you for the comprehensive explanations!
domainMin: { | ||
alignItems: "center", | ||
display: "flex", | ||
width: "calc(((100% - 141px) / 2) + 88.6px)", |
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.
Could you add a comment explaining this calculation here in domainMin
and down below in domainMax
?
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.
Added.
|
||
useEffect(() => { | ||
// Parsing the equation in a "useEffect" hook saves about 2ms each frame | ||
// when the learner is interacting with the graph (i.e. moving points). |
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.
Thank you for this comment!
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.
Thank you for this! 🎉 And for replacing all the instances of it in the other files.
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.
Left some small comments earlier. I trust you'll get to them, so I'll approve the PR now since I'll be OOO tomorrow.
Remove unused code in 'locked-function-settings.tsx'.
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (ffeea6d) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1493 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1493 |
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.
Awesome!! Thank you so much for the updates. This looks sooooo gooooooood, I'm so excited 🎉
expect(onChangeProps).toHaveBeenCalledTimes(3); | ||
expect(onChangeProps).toHaveBeenNthCalledWith(1, {equation: "z"}); | ||
expect(onChangeProps).toHaveBeenNthCalledWith(2, {equation: "o"}); | ||
expect(onChangeProps).toHaveBeenNthCalledWith(3, {equation: "t"}); |
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.
Thank you!
const styles = StyleSheet.create({ | ||
accordionHeader: { | ||
textOverflow: "ellipsis", | ||
maxWidth: "calc(100% - 64px)", |
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.
Thank you for the comprehensive explanations!
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] ### Major Changes - [#1479](#1479) [`ef96cdd54`](ef96cdd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove PropCheckBox component from Perseus; use WB instead ### Minor Changes - [#1455](#1455) [`8b0268215`](8b02682) Thanks [@Myranae](https://github.com/Myranae)! - Update Polygon graphs to have weighted lines only on keyboard focus - [#1492](#1492) [`2ebbc1978`](2ebbc19) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Build the foundation for adding start coords UI for angle graphs - [#1428](#1428) [`eb9f3f9c0`](eb9f3f9) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Drop katex dependency - no longer used - [#1472](#1472) [`4c2ace57a`](4c2ace5) Thanks [@benchristel](https://github.com/benchristel)! - Add keyboard controls to Mafs angle graphs - [#1487](#1487) [`53031f8df`](53031f8) Thanks [@Myranae](https://github.com/Myranae)! - Make graphs non-interactive via keyboard when their question has been answered correctly - [#1486](#1486) [`0b625f560`](0b625f5) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for point graphs - [#1488](#1488) [`0bec013e8`](0bec013) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only) ### Patch Changes - [#1474](#1474) [`a8aaac339`](a8aaac3) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Minor type fixes in the `ImageLoader` and `SvgImage` components - [#1493](#1493) [`de13fd337`](de13fd3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add/Edit/Delete Locked Function in Interactive Graph Editor - [#1476](#1476) [`18f38fca4`](18f38fc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Miscellaneous type improvements - [#1489](#1489) [`de2883b3f`](de2883b) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Correct `hints` type on ItemRenderer - [#1478](#1478) [`7fd586ef4`](7fd586e) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Add "white" as a fill option for locked ellipses and polygons - [#1482](#1482) [`f920a4cc7`](f920a4c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor: Storybook] Add a story for Point graph type start coords - [#1484](#1484) [`808956098`](8089560) Thanks [@handeyeco](https://github.com/handeyeco)! - Minor cleanup around InfoTip - [#1475](#1475) [`6a218bcc1`](6a218bc) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - add non-visual text as a description for expression widget - Updated dependencies \[[`6c6ff52f4`](6c6ff52), [`342a72211`](342a722), [`5e66539e6`](5e66539)]: - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Minor Changes - [#1493](#1493) [`de13fd337`](de13fd3) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Add/Edit/Delete Locked Function in Interactive Graph Editor - [#1492](#1492) [`2ebbc1978`](2ebbc19) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Build the foundation for adding start coords UI for angle graphs - [#1486](#1486) [`0b625f560`](0b625f5) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for point graphs - [#1488](#1488) [`0bec013e8`](0bec013) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Add start coords UI for polygon graphs (snap to grid only) ### Patch Changes - [#1491](#1491) [`395b44f29`](395b44f) Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start Coords] Correct flag logic for polygon graph start coords UI - [#1476](#1476) [`18f38fca4`](18f38fc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Miscellaneous type improvements - [#1479](#1479) [`ef96cdd54`](ef96cdd) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove PropCheckBox component from Perseus; use WB instead - [#1489](#1489) [`de2883b3f`](de2883b) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Correct `hints` type on ItemRenderer - [#1478](#1478) [`7fd586ef4`](7fd586e) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Add "white" as a fill option for locked ellipses and polygons - [#1482](#1482) [`f920a4cc7`](f920a4c) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor: Storybook] Add a story for Point graph type start coords - [#1471](#1471) [`b4e0b60dc`](b4e0b60) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Fix the dropdowns overlapping labels in locked figures settings - [#1481](#1481) [`a35be719f`](a35be71) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Stop page scrolling on number text field focused scroll - [#1484](#1484) [`808956098`](8089560) Thanks [@handeyeco](https://github.com/handeyeco)! - Minor cleanup around InfoTip - [#1477](#1477) [`653b520c6`](653b520) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Migrate ItemExtrasEditor to WB checkboxes - Updated dependencies \[[`a8aaac339`](a8aaac3), [`de13fd337`](de13fd3), [`8b0268215`](8b02682), [`2ebbc1978`](2ebbc19), [`6c6ff52f4`](6c6ff52), [`18f38fca4`](18f38fc), [`ef96cdd54`](ef96cdd), [`342a72211`](342a722), [`de2883b3f`](de2883b), [`7fd586ef4`](7fd586e), [`f920a4cc7`](f920a4c), [`eb9f3f9c0`](eb9f3f9), [`4c2ace57a`](4c2ace5), [`53031f8df`](53031f8), [`808956098`](8089560), [`5e66539e6`](5e66539), [`0b625f560`](0b625f5), [`6a218bcc1`](6a218bc), [`0bec013e8`](0bec013)]: - @khanacademy/[email protected] - @khanacademy/[email protected] ## @khanacademy/[email protected] ### Patch Changes - [#1495](#1495) [`6c6ff52f4`](6c6ff52) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove old buttons that we weren't using anymore - [#1475](#1475) [`342a72211`](342a722) Thanks [@anakaren-rojas](https://github.com/anakaren-rojas)! - update wonder blocks popover versions - [#1496](#1496) [`5e66539e6`](5e66539) Thanks [@handeyeco](https://github.com/handeyeco)! - Remove unused buttons from MathInput; add Lato ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`6c6ff52f4`](6c6ff52), [`342a72211`](342a722), [`5e66539e6`](5e66539)]: - @khanacademy/[email protected] Author: khan-actions-bot Reviewers: Myranae Required Reviewers: Approved By: Myranae Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1473
Summary:
Content editors need a way to add and edit a locked function. This PR adds a settings section that is specific to functions.
Issue: LEMS-1947
Test plan:
Affected UI
New settings options