[Interactive Graph] Ensure we have defaults for labels in our Locked Figures

[Evelas78]

Summary:

Ensure defaults exist in both our parser and editor logic

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

Test plan:

• Verify using jest tests to confirm that the default is detected correctly when color and size is passed in as undefined

[github-actions]

Size Change: +29 B (+0.01%)

Total Size: 513 kB

📦 View Changed

Filename	Size	Change
packages/perseus-core/dist/es/index.item-splitting.js	12.3 kB	+15 B (+0.12%)
packages/perseus-core/dist/es/index.js	27.3 kB	+14 B (+0.05%)

ℹ️ View Unchanged

Filename	Size
packages/kas/dist/es/index.js	20.6 kB
packages/keypad-context/dist/es/index.js	1 kB
packages/kmath/dist/es/index.js	6.32 kB
packages/math-input/dist/es/index.js	98.5 kB
packages/math-input/dist/es/strings.js	1.61 kB
packages/perseus-editor/dist/es/index.js	105 kB
packages/perseus-linter/dist/es/index.js	9.8 kB
packages/perseus-score/dist/es/index.js	10.2 kB
packages/perseus-utils/dist/es/index.js	403 B
packages/perseus/dist/es/index.js	203 kB
packages/perseus/dist/es/strings.js	8.6 kB
packages/pure-markdown/dist/es/index.js	1.39 kB
packages/simple-markdown/dist/es/index.js	6.71 kB

_{compressed-size-action}

[github-actions]

npm Snapshot: Published

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

Example:

pnpm add @khanacademy/perseus@PR3765

If you are working in Khan Academy's frontend, you can run the below command.

./dev/tools/bump_perseus_version.ts -t PR3765

If you are working in Khan Academy's webapp, you can run the below command.

./dev/tools/bump_perseus_version.js -t PR3765

[jeremywiebe]

This default doesn't feel very helpful. To me, this signals that we're trying to provide a default, but the problem is actually somewhere else (namely, the editor is allowing folks to save a locked figure with missing info).

[Evelas78]

Completely agreed, if I recall, I think I set it to "label" as get-default-figure-for-type.ts also sets it to, I was thinking of maybe changing it to "label not set", something that makes it clear that it isn't something that should be done (unless our aim would be to block saving when it isn't set, though that may be out of scope for this ticket)

What do you think?

[nishasy]

I think the reason I made the labels default to "label" is because it was initially "" and then it wasn't obvious if someone created a label but didn't populate it. Tbh I feel like it doesn't matter much what the default is either way, but I don't think it's necessary to update it to "label not set." Content authors are competent and know that seeing "label" on an interactive graph means they haven't set the label.

[nishasy]

Actually now that I've said that, I'm starting to think we shouldn't have a label default. If we have undefined/empty labels anywhere in our content, and we update the parser so that it's a string now, there could be rogue labels showing up in existing interactive graphs.

[jeremywiebe]

Defaults should really only be provided when we can provide a sensible default. Is the text property not required? If it isn't, then we should leave it undefined. If it is, perhaps a "" is better?

[nishasy]

It doesn't make any sense to have a locked label with no text. The text field is required in the schema.

[jeremywiebe]

Tbh I feel like it doesn't matter much what the default is either way, but I don't think it's necessary to update it to "label not set." Content authors are competent and know that seeing "label" on an interactive graph means they haven't set the label.

We use the parser for learner-facing parsing too, so what happens if we hit this on the learner-side and we default something like "label"?

[nishasy]

@jeremywiebe I'm worried about that case. That's what I meant with my earlier comment

Actually now that I've said that, I'm starting to think we shouldn't have a label default. If we have undefined/empty labels anywhere in our content, and we update the parser so that it's a string now, there could be rogue labels showing up in existing interactive graphs.

If we have a locked label in an existing graph, and for some reason the label is saved in the IG data as an empty string or something, we wouldn't want it to suddenly start showing up as "label"

They're already defaulting to "label" at creation time. I'm not sure there's a reason the parser needs to default the label text. Maybe if it's undefined somehow when it's typed as a required string, we should default it to "" in the parser?

[Evelas78]

I think I agree with the idea of defaulting to "" in the parser and having a default undefined in the get-default file, though will wait for like a clear conclusion before making the change

[nishasy]

Thanks for adding the regression test!

[nishasy]

Nice!! Thanks for the updates. Looks good to me 🎉