Skip to content
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

Remove PropCheckBox #1479

Merged
merged 15 commits into from
Aug 5, 2024
Merged

Remove PropCheckBox #1479

merged 15 commits into from
Aug 5, 2024

Conversation

handeyeco
Copy link
Contributor

@handeyeco handeyeco commented Aug 2, 2024

Summary:

While looking at WB-ifying ExpressionEditor I noticed this weird component called PropCheckBox and thought we should just switch to WB Checkbox because:

  • We're trying to offload work to WB when possible
  • PropCheckBox was using ancient createReactClass
  • It wasn't properly typed
  • IMO what it was doing was a little too cute

Issue: Part of LEMS-2222

Test plan:

Nothing should change, tests should all pass

@handeyeco handeyeco self-assigned this Aug 2, 2024
@khan-actions-bot khan-actions-bot requested a review from a team August 2, 2024 18:39
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented Aug 2, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/four-windows-retire.md, packages/perseus/src/components.ts, packages/perseus-editor/src/components/graph-settings.tsx, packages/perseus-editor/src/components/interactive-graph-settings.tsx, packages/perseus-editor/src/widgets/categorizer-editor.tsx, packages/perseus-editor/src/widgets/cs-program-editor.tsx, packages/perseus-editor/src/widgets/expression-editor.tsx, packages/perseus-editor/src/widgets/iframe-editor.tsx, packages/perseus-editor/src/widgets/matcher-editor.tsx, packages/perseus-editor/src/widgets/measurer-editor.tsx, packages/perseus-editor/src/widgets/number-line-editor.tsx, packages/perseus-editor/src/widgets/numeric-input-editor.tsx, packages/perseus-editor/src/widgets/passage-editor.tsx, packages/perseus-editor/src/widgets/sorter-editor.tsx, packages/perseus-editor/src/widgets/__tests__/numeric-input-editor.test.tsx, packages/perseus-editor/src/widgets/radio/editor.tsx

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

@handeyeco handeyeco changed the title rm from interactive-graph-settings Remove PropCheckBox Aug 2, 2024
Copy link
Contributor

github-actions bot commented Aug 2, 2024

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1479

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1479

Copy link
Contributor

github-actions bot commented Aug 2, 2024

Size Change: -579 B (-0.07%)

Total Size: 852 kB

Filename Size Change
packages/perseus-editor/dist/es/index.js 272 kB -40 B (-0.01%)
packages/perseus/dist/es/index.js 412 kB -539 B (-0.13%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 38.3 kB
packages/kmath/dist/es/index.js 4.27 kB
packages/math-input/dist/es/index.js 80.8 kB
packages/math-input/dist/es/strings.js 1.73 kB
packages/perseus-core/dist/es/index.js 1.48 kB
packages/perseus-linter/dist/es/index.js 21.6 kB
packages/perseus/dist/es/strings.js 3.23 kB
packages/pure-markdown/dist/es/index.js 3.67 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

@handeyeco handeyeco requested a review from a team August 2, 2024 18:45
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 71.92982% with 48 lines in your changes missing coverage. Please review.

Project coverage is 70.65%. Comparing base (a8aaac3) to head (bb07c8a).
Report is 1 commits behind head on main.

Files Patch % Lines
...s/perseus-editor/src/widgets/cs-program-editor.tsx 16.66% 10 Missing ⚠️
...kages/perseus-editor/src/widgets/iframe-editor.tsx 16.66% 10 Missing ⚠️
...ges/perseus-editor/src/widgets/measurer-editor.tsx 16.66% 10 Missing ⚠️
...ckages/perseus-editor/src/widgets/radio/editor.tsx 66.66% 7 Missing ⚠️
...ages/perseus-editor/src/widgets/passage-editor.tsx 28.57% 5 Missing ⚠️
...s/perseus-editor/src/components/graph-settings.tsx 76.47% 4 Missing ⚠️
...itor/src/components/interactive-graph-settings.tsx 83.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1479      +/-   ##
==========================================
+ Coverage   70.05%   70.65%   +0.60%     
==========================================
  Files         509      511       +2     
  Lines      105435   105469      +34     
  Branches     5351    11443    +6092     
==========================================
+ Hits        73858    74520     +662     
+ Misses      31460    30949     -511     
+ Partials      117        0     -117     

Impacted file tree graph

Files Coverage Δ
.../perseus-editor/src/widgets/categorizer-editor.tsx 80.18% <100.00%> (+19.61%) ⬆️
...s/perseus-editor/src/widgets/expression-editor.tsx 73.67% <100.00%> (-9.55%) ⬇️
...ages/perseus-editor/src/widgets/matcher-editor.tsx 68.83% <100.00%> (+1.04%) ⬆️
.../perseus-editor/src/widgets/number-line-editor.tsx 80.30% <100.00%> (+0.23%) ⬆️
...erseus-editor/src/widgets/numeric-input-editor.tsx 77.50% <100.00%> (+0.18%) ⬆️
...kages/perseus-editor/src/widgets/sorter-editor.tsx 97.16% <100.00%> (+17.55%) ⬆️
packages/perseus/src/components.ts 100.00% <ø> (ø)
...itor/src/components/interactive-graph-settings.tsx 98.59% <83.33%> (-0.31%) ⬇️
...s/perseus-editor/src/components/graph-settings.tsx 97.24% <76.47%> (-0.59%) ⬇️
...ages/perseus-editor/src/widgets/passage-editor.tsx 15.96% <28.57%> (+0.58%) ⬆️
... and 4 more

... and 126 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 a8aaac3...bb07c8a. Read the comment docs.

checked={this.props.showTooltips}
onChange={(value) => {
this.change({showTooltips: value});
}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: It seems like this might not have had coverage before, but it might be easy to cover with something like the following added to the graph-settings.test.tsx file. Looks like there are a number of places marked as missing coverage. Probably not necessary to fix all the coverage, but here's a possible test here at least:

test("calls onChange when tooltips label is changed", async () => {
        // Arrange
        const onChange = jest.fn();
        render(
            <GraphSettings
                editableSettings={["graph"]}
                showTooltips={true}
                onChange={onChange}
            />,
        );

        // Act
        const checkbox = screen.getByRole("checkbox", {
            name: "Show tooltips",
        });
        await userEvent.click(checkbox);

        // Assert
        expect(onChange).toHaveBeenCalledWith(
            expect.objectContaining({showTooltips: false}),
            undefined,
        );
    });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! I think I'm going to defer to the interactive-graph team to add tests in a separate PR if they find them useful. Happy to do a review though.

Copy link
Contributor

@Myranae Myranae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Love moving anything over to WB instead of custom. Was able to manually check radio, interactive graph, number input, number line, and expression in storybook. The checkboxes seem to work as intended. Tried sorter and categorizer, but the stories don't seem to work. Number line tooltips don't work on prod or this branch, so that's fine XD Let me know if you'd like me to also manually check the rest, but seems good as they all follow a similar pattern.

@handeyeco handeyeco merged commit ef96cdd into main Aug 5, 2024
11 checks passed
@handeyeco handeyeco deleted the rm-prop-check-box branch August 5, 2024 19:16
Myranae pushed a commit that referenced this pull request Aug 9, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants