Show error if locked line has length 0

[nishasy]

Summary:

It doesn't make sense for a locked line to have length 0, so
I'm adding an error message if the user were to set the two
points to the same value.

Implementation notes/questions:

• I really tried to use WB TextField validate for this, but it
  was extremely convoluted to get the four text fieds to stay in
  sync. Or even to keep two of them in sync. If it has the error
  in one, but you update it in the other point, it would just
  not update the original error. I sort of used a manual state
  for this instead and replicated the style. It's much easier
  and cleaner this way.
• This implementation does not stop the graph from receiving
  a line with the same two defining points. Is that something
  worth doing? I'd have to change where the error gets set,
  but it's doable.

Issue: https://khanacademy.atlassian.net/browse/LEMS-1991

Test plan:

yarn jest packages/perseus-editor/src/components/__tests__/locked-line-settings.test.tsx

Storybook
http://localhost:6006/?path=/story/perseuseditor-components-locked-line-settings--controlled

Demo

Screen.Recording.2024-05-15.at.3.50.56.PM.mov

[github-actions]

Size Change: +119 B (+0.01%)

Total Size: 839 kB

Filename	Size	Change
packages/perseus-editor/dist/es/index.js	268 kB	+143 B (+0.05%)
packages/perseus/dist/es/index.js	403 kB	-24 B (-0.01%)

ℹ️ 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	80.5 kB
packages/math-input/dist/es/strings.js	1.73 kB
packages/perseus-core/dist/es/index.js	908 B
packages/perseus-error/dist/es/index.js	877 B
packages/perseus-linter/dist/es/index.js	21.8 kB
packages/perseus/dist/es/strings.js	3.22 kB
packages/pure-markdown/dist/es/index.js	3.68 kB
packages/simple-markdown/dist/es/index.js	12.4 kB

_{compressed-size-action}

[khan-actions-bot]

Gerald

Required Reviewers

• @Khan/perseus for changes to .changeset/thirty-bugs-march.md, packages/perseus-editor/src/components/coordinate-pair-input.tsx, packages/perseus-editor/src/components/defining-point-settings.tsx, packages/perseus-editor/src/components/locked-line-settings.tsx, packages/perseus-editor/src/components/__stories__/locked-line-settings.stories.tsx, packages/perseus-editor/src/components/__tests__/locked-line-settings.test.tsx

---

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.28%. Comparing base (e14a003) to head (c3a157e).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1277      +/-   ##
==========================================
+ Coverage   68.81%   70.28%   +1.47%     
==========================================
  Files         477      481       +4     
  Lines      101842   101960     +118     
  Branches     7213    10300    +3087     
==========================================
+ Hits        70082    71667    +1585     
+ Misses      31581    30293    -1288     
+ Partials      179        0     -179     

Files	Coverage Δ
...us-editor/src/components/coordinate-pair-input.tsx	100.00% <100.00%> (ø)
...-editor/src/components/defining-point-settings.tsx	100.00% <100.00%> (+9.16%)	⬆️
...eus-editor/src/components/locked-line-settings.tsx	100.00% <100.00%> (ø)

... and 141 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 e14a003...c3a157e. Read the comment docs.

[jeremywiebe]

I don't have any concerns with the code, but showing 2 errors for the same issue feels off to me. It also causes quite a bit of junk as they are shown/hidden. I wonder if we could put the error above the two points. It really is an issue with the line and not the individual locked points.

Also, the editor system has a way to generate errors to block saving via the getSaveWarnings() function. Any widget editor can implement that and return an array of strings representing the issues. Webapp will then prevent saving if the top-level function returns any issues. I'd recommend doing that here so that we do more than simply render an error, we actually block saving.

[nishasy]

@jeremywiebe

showing 2 errors for the same issue feels off to me. It also causes quite a bit of junk as they are shown/hidden. I wonder if we could put the error above the two points

That makes sense to me. I'll move the error. Do you think it still makes sense to highlight those four text fields in red so it's apparent which inputs are related to the error?

[jeremywiebe]

@jeremywiebe

showing 2 errors for the same issue feels off to me. It also causes quite a bit of junk as they are shown/hidden. I wonder if we could put the error above the two points

That makes sense to me. I'll move the error. Do you think it still makes sense to highlight those four text fields in red so it's apparent which inputs are related to the error?

Nice. I think that's much better. It still highlights the fields that contribute to the error state, but it's not so "loud". :)

[github-actions]

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1277

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

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

[benchristel]

LGTM! Just one question inline about i18n.

[benchristel]

This string doesn't need to be added to PerseusStrings for translation, does it? Do we have a policy of not doing i18n for the editor?

[nishasy]

I did a quick search, and it doesn't look like any of the editor strings are translated anywhere else.

[jeremywiebe]

Confirmed. We don't translate the content editor.