Plotter defaults #2148
Plotter defaults #2148
Conversation
| // @ts-expect-error - TS2322 | ||
| get picUrl() { | ||
| const staticUrl = Dependencies.getDependencies().staticUrl; | ||
| if (staticUrl) { | ||
| return staticUrl("/images/badges/earth-small.png"); | ||
| } | ||
|
|
||
| return null; | ||
| }, |
There was a problem hiding this comment.
I just felt like this was a little much just to have a placeholder image.
There was a problem hiding this comment.
I've always really disliked that default props has behaviour embedded in it via a getter!
What does the Plotter show by default in the editor with this now defaulting to null.
There was a problem hiding this comment.
Turns out this was kind of a hard question to answer.
- I added a story, but SB didn't properly resolve the URL for the image; it looked the same in
mainand this branch - I tried to look in the content editor, but Plotter is a hidden widget so I couldn't make new widget to test
I think I might just merge this PR since the widget is hidden anyway. Shouldn't change the editing experience since they'll never see an empty Plotter widget right?
There was a problem hiding this comment.
I think I might just merge this PR since the widget is hidden anyway. Shouldn't change the editing experience since they'll never see an empty Plotter widget right?
Good point! For hidden widgets, they can never be "created" in the editor anyways, so defaults for these widgets feel like a fading concern.
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (02a39bc) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2148If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2148 |
|
Size Change: -39 B (0%) Total Size: 1.48 MB
ℹ️ View Unchanged
|
…ic to Perseus Core
|
|
||
| picSize: 30, | ||
| picBoxHeight: 36, | ||
| plotDimensions: [275, 200], |
There was a problem hiding this comment.
Woah, that's some random size. 😂 Our graphs are usually 400x400.
No action needed.
| // @ts-expect-error - TS2322 | ||
| get picUrl() { | ||
| const staticUrl = Dependencies.getDependencies().staticUrl; | ||
| if (staticUrl) { | ||
| return staticUrl("/images/badges/earth-small.png"); | ||
| } | ||
|
|
||
| return null; | ||
| }, |
There was a problem hiding this comment.
I've always really disliked that default props has behaviour embedded in it via a getter!
What does the Plotter show by default in the editor with this now defaulting to null.
Summary:
This actually includes a logic change,
get picUrlseemed over-engineered and unnecessary.Issue: LEMS-2737
Test plan:
Plotter can be edited, Plotters edited with this change can be rendered.