Update graph-settings.tsx to React class component #1009

nishasy · 2024-02-20T23:40:56Z

Summary:

Update GraphSettings to be a React class instead of old createReactClass const.
Add types

Tests coming in follow-up PR.

Issue: https://khanacademy.atlassian.net/browse/LC-1702

Test plan:

yarn jest
yarn typecheck

todo Issue: https://khanacademy.atlassian.net/browse/LC-1702 Test plan: todo

github-actions · 2024-02-20T23:43:38Z

Size Change: -91 B (0%)

Total Size: 815 kB

Filename	Size	Change
`packages/perseus-editor/dist/es/index.js`	263 kB	-91 B (0%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.1 kB
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	79.7 kB
`packages/perseus-core/dist/es/index.js`	908 B
`packages/perseus-error/dist/es/index.js`	878 B
`packages/perseus-linter/dist/es/index.js`	21.8 kB
`packages/perseus/dist/es/index.js`	390 kB
`packages/pure-markdown/dist/es/index.js`	3.77 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

codecov · 2024-02-20T23:46:12Z

Codecov Report

Attention: 85 lines in your changes are missing coverage. Please review.

Comparison is base (6ef53d2) 63.65% compared to head (2156ef4) 64.85%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1009      +/-   ##
==========================================
+ Coverage   63.65%   64.85%   +1.19%     
==========================================
  Files         425      427       +2     
  Lines       96484    96523      +39     
  Branches     6281     8678    +2397     
==========================================
+ Hits        61420    62603    +1183     
+ Misses      35064    33920    -1144

Files	Coverage Δ
...us-editor/src/widgets/interactive-graph-editor.tsx	`17.51% <100.00%> (ø)`
packages/perseus/src/components/graph.tsx	`73.94% <100.00%> (+9.11%)`	⬆️
packages/perseus/src/widgets/grapher/util.tsx	`62.20% <100.00%> (+12.64%)`	⬆️
.../widgets/interaction-editor/interaction-editor.tsx	`8.75% <10.00%> (-0.06%)`	⬇️
...s/perseus-editor/src/components/graph-settings.tsx	`12.46% <41.98%> (-11.47%)`	⬇️

... and 76 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ef53d2...2156ef4. Read the comment docs.

khan-actions-bot · 2024-02-21T00:47:52Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/friendly-trees-drive.md, packages/perseus/src/perseus-types.ts, packages/perseus/src/components/graph.tsx, packages/perseus-editor/src/components/graph-settings.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor.tsx, packages/perseus/src/widgets/grapher/util.tsx, packages/perseus-editor/src/widgets/interaction-editor/interaction-editor.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

jeremywiebe

Awesome! Thanks.

jeremywiebe · 2024-02-21T18:21:20Z

packages/perseus-editor/src/components/graph-settings.tsx


-    stateFromProps: function (props) {
+    stateFromProps(props) {


Can you mark this as static? It will mean a few changes to call sites, but it'll ensure we don't "cheat" in this function ever and start accessing instance fields.

jeremywiebe · 2024-02-21T18:23:55Z

packages/perseus-editor/src/widgets/interaction-editor/interaction-editor.tsx

+                        box={this.props.graph.box as [number, number]}
                        labels={this.props.graph.labels}
-                        range={this.props.graph.range}
-                        step={this.props.graph.tickStep}
-                        gridStep={this.props.graph.gridStep}
+                        range={
+                            this.props.graph.range as [
+                                [number, number],
+                                [number, number],
+                            ]
+                        }
+                        step={this.props.graph.tickStep as [number, number]}
+                        gridStep={this.props.graph.gridStep as [number, number]}


suggestion: In the next (or near future) PR you could change this components props so that they use tuples instead of ReadonlyArray<>. That'd eliminate the need to do the casting you're doing here... and I think it's more accurate.

I think the props are already tuples. The problem is that we're doing a slice() on them just above this, which is causing it to become number[] types. And then I get a type error because that means it can be shorter than the tuple expects.

I can move the as [number, number] to those lines instead?

Actually, all those .slice() calls look like they're just used to copy the array. If you switch to spreading the array, Typescript seems to retain that their a tuple and you can eliminate the as casting entirely.

https://www.typescriptlang.org/play?#code/C4TwDgpgBAggRgYygXigbwFBW1ATgQwDsBzCALigG1CBXAWzglwBopaGmBdAbgwF9eGBAHtCAZ2BQAHhXhJUmHHiKkKlAIwAGVls78MQ0RKghZiFOiw4CJclQB0jqfZuk9fIA

Note: you are welcome to land this PR as is and do these changes in a new PR. Whatever works for you. :)

github-actions · 2024-02-21T18:37:22Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (2156ef4) and published it to npm. You
can install it using the tag PR1009.

Example:

yarn add @khanacademy/perseus@PR1009

This reverts commit 7c030e6.

…#1020) 🖍 _This is an audit!_ 🖍 ## Summary: Revert "Update graph-settings.tsx to React class component (#1009)" Reverting because this caused `this.setState` errors in the exercise editor. Fix for the future: update the functions in GraphSettings to arrow functions. Issue: XXX-XXXX ## Test plan: Author: nishasy Auditors: jeremywiebe Required Reviewers: Approved By: jeremywiebe Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage, ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Extract i18n strings (ubuntu-latest, 20.x), ✅ 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), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ✅ Cypress (ubuntu-latest, 20.x) Pull Request URL: #1020

## Summary: This PR is pretty much the same as #1009. The changes from that PR got rolled back, and now interactive-graph-settings.tsx has been split out as a copy of graph-settings.tsx. Instead of updating graph-settings.tsx, we're keeping that as is as it will only be used by deprecated components. interactive-graph-settings.tsx will continue to be updated along with InteractiveGraph. To make these updates easier, we're making InteractiveGraphSettings a React class component instead of it using the old `createReactClass` const. - Update InteractiveGraphSettings to be a React class - Add types - Use arrow functions so `this` binds correctly Additional cleanup coming in a following PR. Issue: https://khanacademy.atlassian.net/browse/LC-1702 ## Test plan: `yarn jest packages/perseus-editor/src/components/__tests__/interactive-graph-settings.test.tsx` `yarn typecheck` Author: nishasy Reviewers: jeremywiebe, nishasy, nedredmond Required Reviewers: Approved By: jeremywiebe Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage, ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Extract i18n strings (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ gerald, ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x) Pull Request URL: #1033

Update graph-settings.tsx to React class component

99d7423

todo Issue: https://khanacademy.atlassian.net/browse/LC-1702 Test plan: todo

nishasy self-assigned this Feb 20, 2024

nishasy requested a review from jeremywiebe February 20, 2024 23:46

Add changeset. Formatting.

1e69b6d

nishasy marked this pull request as ready for review February 21, 2024 00:47

khan-actions-bot requested a review from a team February 21, 2024 00:47

jeremywiebe approved these changes Feb 21, 2024

View reviewed changes

Make stateFromProps staic

2156ef4

nishasy merged commit 7c030e6 into main Feb 21, 2024
12 of 13 checks passed

nishasy deleted the modernize-graph-settings branch February 21, 2024 18:49

khan-actions-bot mentioned this pull request Feb 21, 2024

Version Packages #1010

Merged

nishasy added a commit that referenced this pull request Feb 23, 2024

Revert "Update graph-settings.tsx to React class component (#1009)"

28b7c8c

This reverts commit 7c030e6.

nishasy mentioned this pull request Feb 23, 2024

[🔥AUDIT🔥] Revert "Update graph-settings.tsx to React class component" #1020

Merged

nishasy mentioned this pull request Feb 27, 2024

Update interactive-graph-settings.tsx to React class component #1033

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update graph-settings.tsx to React class component #1009

Update graph-settings.tsx to React class component #1009

nishasy commented Feb 20, 2024 •

edited

Loading

github-actions bot commented Feb 20, 2024 •

edited

Loading

codecov bot commented Feb 20, 2024 •

edited

Loading

khan-actions-bot commented Feb 21, 2024

jeremywiebe left a comment

jeremywiebe Feb 21, 2024

jeremywiebe Feb 21, 2024

nishasy Feb 21, 2024

nishasy Feb 21, 2024

jeremywiebe Feb 21, 2024

github-actions bot commented Feb 21, 2024

Update graph-settings.tsx to React class component #1009

Update graph-settings.tsx to React class component #1009

Conversation

nishasy commented Feb 20, 2024 • edited Loading

Summary:

Test plan:

github-actions bot commented Feb 20, 2024 • edited Loading

codecov bot commented Feb 20, 2024 • edited Loading

Codecov Report

khan-actions-bot commented Feb 21, 2024

Gerald

jeremywiebe left a comment

Choose a reason for hiding this comment

jeremywiebe Feb 21, 2024

Choose a reason for hiding this comment

jeremywiebe Feb 21, 2024

Choose a reason for hiding this comment

nishasy Feb 21, 2024

Choose a reason for hiding this comment

nishasy Feb 21, 2024

Choose a reason for hiding this comment

jeremywiebe Feb 21, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2024

npm Snapshot: Published

nishasy commented Feb 20, 2024 •

edited

Loading

github-actions bot commented Feb 20, 2024 •

edited

Loading

codecov bot commented Feb 20, 2024 •

edited

Loading