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

[Hint Mode: Start Coords] Add separate flags for graph types #1465

Merged
merged 4 commits into from
Jul 31, 2024

Conversation

nishasy
Copy link
Contributor

@nishasy nishasy commented Jul 30, 2024

Summary:

We want to be able to release the start coords UI even if
only a subset of graph types are ready. To accomodate this,
I'm adding individual flags for each graph type.

Issue: none

Test plan:

Storybook

We want to be able to release the start coords UI even if
only a subset of graph types are ready. To accomodate this,
I'm adding individual flags for each graph type.

Issue: none

Test plan:
Storybook
- http://localhost:6006/?path=/docs/perseuseditor-widgets-interactive-graph--docs
- Confirm that the UI shows up for the currently landed graphs - segment, ray, linear, linear-system, circle
- Confirm that the UI does not show up for the graphs that are not done yet - sinusoid, quadratic, angle, polygon, point
@nishasy nishasy self-assigned this Jul 30, 2024
Copy link
Contributor

github-actions bot commented Jul 30, 2024

Size Change: +1.04 kB (+0.12%)

Total Size: 851 kB

Filename Size Change
packages/math-input/dist/es/index.js 80.8 kB +45 B (+0.06%)
packages/perseus-editor/dist/es/index.js 271 kB +449 B (+0.17%)
packages/perseus/dist/es/index.js 412 kB +530 B (+0.13%)
packages/perseus/dist/es/strings.js 3.23 kB +17 B (+0.53%)
ℹ️ 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/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/pure-markdown/dist/es/index.js 3.67 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

Copy link

codecov bot commented Jul 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.43%. Comparing base (79a09d6) to head (5351887).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1465      +/-   ##
==========================================
+ Coverage   69.60%   70.43%   +0.83%     
==========================================
  Files         507      510       +3     
  Lines      105019   105194     +175     
  Branches     7562    11408    +3846     
==========================================
+ Hits        73099    74095     +996     
+ Misses      31731    31099     -632     
+ Partials      189        0     -189     

Impacted file tree graph

Files Coverage Δ
...us-editor/src/widgets/interactive-graph-editor.tsx 91.42% <100.00%> (+0.17%) ⬆️

... and 158 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 79a09d6...5351887. Read the comment docs.

@@ -473,7 +473,7 @@ class InteractiveGraphEditor extends React.Component<Props> {
</LabeledRow>
)}
{this.props.graph?.type &&
this.props.apiOptions?.flags?.mafs?.["start-coords-ui"] && (
(this.props.apiOptions?.flags?.mafs?.["start-coords-ui"][this.props.graph.type]) && (
Copy link
Member

@benchristel benchristel Jul 31, 2024

Choose a reason for hiding this comment

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

I think this needs to be

Suggested change
(this.props.apiOptions?.flags?.mafs?.["start-coords-ui"][this.props.graph.type]) && (
(this.props.apiOptions?.flags?.mafs?.["start-coords-ui"]?.[this.props.graph.type]) && (

or it will throw an error when mafs["start-coords-ui"] is undefined.

EDIT: oh, I see that ["start-coords-ui"] is a required property. I guess this is okay, then, because TypeScript will make sure start-coords-ui is always an object when the flags are set in webapp.

Copy link
Member

@benchristel benchristel left a comment

Choose a reason for hiding this comment

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

This looks good to me. I kind of wonder if it will be hard to manage all these different flags, e.g. to know which ones are turned on in Growthbook, turn them on for everyone who needs to playtest, etc. I might prefer to have milestone-based flags like we do for locked features. But I'm okay with it either way.

@nishasy nishasy marked this pull request as ready for review July 31, 2024 17:47
@khan-actions-bot
Copy link
Contributor

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/slow-taxis-give.md, packages/perseus/src/types.ts, packages/perseus-editor/src/__stories__/flags-for-api-options.ts, packages/perseus-editor/src/__stories__/interactive-graph-editor.stories.tsx, packages/perseus-editor/src/widgets/interactive-graph-editor.tsx, packages/perseus-editor/src/widgets/__tests__/interactive-graph-editor.test.tsx

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

@nishasy nishasy requested a review from benchristel July 31, 2024 17:48
Copy link
Member

@benchristel benchristel left a comment

Choose a reason for hiding this comment

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

LGTM!

@nishasy nishasy merged commit 94ad04f into main Jul 31, 2024
12 checks passed
@nishasy nishasy deleted the start-coords-ui-flag branch July 31, 2024 17:54
nishasy added a commit that referenced this pull request Aug 1, 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]

### Minor Changes

- [#1468](#1468)
[`af68a9e08`](af68a9e)
Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start
Coords] Add start coords UI for sinusoid graphs


- [#1469](#1469)
[`6e1ec850c`](6e1ec85)
Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start
Coords] Add start coords UI for quadratic graphs

### Patch Changes

- [#1470](#1470)
[`942b0a9a5`](942b0a9)
Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph
Locked Figures] Remove m2 flag from the code


- [#1465](#1465)
[`94ad04fee`](94ad04f)
Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start
Coords] Add separate flags for graph types


- [#1432](#1432)
[`ed6737025`](ed67370)
Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Bug
fix to ensure that new angle graphs are scored correctly.

## @khanacademy/[email protected]

### Minor Changes

- [#1468](#1468)
[`af68a9e08`](af68a9e)
Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start
Coords] Add start coords UI for sinusoid graphs


- [#1469](#1469)
[`6e1ec850c`](6e1ec85)
Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start
Coords] Add start coords UI for quadratic graphs

### Patch Changes

- [#1470](#1470)
[`942b0a9a5`](942b0a9)
Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph
Locked Figures] Remove m2 flag from the code


- [#1465](#1465)
[`94ad04fee`](94ad04f)
Thanks [@nishasy](https://github.com/nishasy)! - [Hint Mode: Start
Coords] Add separate flags for graph types

- Updated dependencies
\[[`af68a9e08`](af68a9e),
[`942b0a9a5`](942b0a9),
[`6e1ec850c`](6e1ec85),
[`94ad04fee`](94ad04f),
[`ed6737025`](ed67370)]:
    -   @khanacademy/[email protected]
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