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.

Sign up for GitHub

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

Jump to bottom

Angle Graph bug fixes and refactoring #1666

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Angle Graph bug fixes and refactoring #1666

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5442dd2
Initial work / prototype before switching tickets
SonicScrewdriver Sep 16, 2024
9f10c0a
Refactoring of all angle methods to be as dry and reusable as possible
SonicScrewdriver Sep 25, 2024
69a54d0
More consistent naming
SonicScrewdriver Sep 26, 2024
54f9d74
Prettier
SonicScrewdriver Sep 26, 2024
4350c0f
Updating tests
SonicScrewdriver Sep 26, 2024
7052785
Fixing a miss during the rebase
SonicScrewdriver Oct 1, 2024
90aabd2
PR Feedback
SonicScrewdriver Oct 3, 2024
61fa24f
Cleanup is tons of fun
SonicScrewdriver Oct 4, 2024
86fef16
adding test
SonicScrewdriver Oct 4, 2024
f8414f8
PR Feedback
SonicScrewdriver Oct 4, 2024
1a10f10
We no longer need to specify false as it's the default
SonicScrewdriver Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
PR Feedback
  • Loading branch information
@SonicScrewdriver
SonicScrewdriver committed Oct 4, 2024
commit f8414f8a1eecb776fa3c125ed17f3a9a6eaf2f99
62 changes: 30 additions & 32 deletions packages/perseus-editor/src/widgets/interactive-graph-editor/interactive-graph-editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ class InteractiveGraphEditor extends React.Component<Props> {
// Don't show indeterminate checkbox state
!!this.props.correct?.showAngles
}
onChange={() => {
onChange={(newValue) => {
if (this.props.graph?.type === "angle") {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit since this seems to be the pattern in the file:
could the logic for the checkboxes be extracted into their own methods?

    handleAngleChange = () => {
        const { graph, correct, onChange } = this.props;
        if (graph?.type !== "angle") return;

        invariant(
            correct.type === "angle",
            `Expected graph type to be angle, but got ${correct.type}`,
        );

        onChange({
            correct: {
                ...correct,
                showAngles: !correct.showAngles,
            },
            graph: {
                ...graph,
                showAngles: !graph.showAngles,
            },
        });
    };
    // then in the checkbox
      <Checkbox
          ...
          onChange={this.handleAngleChange}
      />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooo good callout! They definitely could be separated out, but I think this PR is probably already large and long-lived enough as it is.😅

That sounds like a good clean up ticket for Interactive Graph's backlog however! It seems like this is the existing design pattern in this file, and we should probably update all of the checkbox logic together in order to maintain consistency.

invariant(
this.props.correct.type === "angle",
Expand All @@ -425,15 +425,11 @@ class InteractiveGraphEditor extends React.Component<Props> {
this.props.onChange({
correct: {
...this.props.correct,
showAngles:
!this.props.correct
.showAngles,
showAngles: newValue,
},
graph: {
...this.props.graph,
showAngles:
!this.props.graph
.showAngles,
showAngles: newValue,
},
});
}
Expand All @@ -446,40 +442,42 @@ class InteractiveGraphEditor extends React.Component<Props> {
<View style={styles.row}>
<Checkbox
label={
<LabelSmall>
Allow reflexive angles
</LabelSmall>
<LabelSmall>Allow reflex angles</LabelSmall>
}
checked={
// Don't show indeterminate checkbox state
!!this.props.correct?.allowReflexAngles
}
onChange={() => {
if (
this.props.graph?.type === "angle" &&
this.props.correct.type === "angle"
) {
this.props.onChange({
correct: {
...this.props.correct,
allowReflexAngles:
!this.props.correct
.allowReflexAngles,
},
graph: {
...this.props.graph,
allowReflexAngles:
!this.props.graph
.allowReflexAngles,
},
});
}
onChange={(newValue) => {
invariant(
this.props.correct.type === "angle",
`Expected graph type to be angle, but got ${this.props.correct.type}`,
);
invariant(
this.props.graph?.type === "angle",
`Expected graph type to be angle, but got ${this.props.graph?.type}`,
);

const update = {
allowReflexAngles: newValue,
};

this.props.onChange({
correct: {
...this.props.correct,
...update,
},
graph: {
...this.props.graph,
...update,
},
});
}}
/>
<InfoTip>
<p>
Allow students to be able to create
reflexive angles.
Allow students to be able to create reflex
angles.
</p>
</InfoTip>
</View>
Expand Down
2 changes: 1 addition & 1 deletion packages/perseus/src/util/graphie.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1772,7 +1772,7 @@ const GraphUtils = {

// Find the angle in degrees between two or three points
// This function is deprecated as it has several issues calculating
// correctly when dealing with reflexive angles or the position of point2
// correctly when dealing with reflex angles or the position of point2
// (LEMS-2202) Remove this function while removing the Legacy Interactive Graph.
findAngleDeprecated: function (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add a comment to LEMS-2202 to mention deleting this function once we strip out the Legacy Graph. First, I need to confirm if interactive.ts is used anywhere else as it's the only other consumer of this function.

Copy link
Contributor

@Myranae Myranae Oct 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Praise: Thanks for making it clear this function should not be used in the future!

Question: Are angles measured incorrectly where this is still used?

point1: Coord,
Expand Down