Fix: Image widget editor fails to calculate image dimensions #618
Conversation
🦋 Changeset detectedLatest commit: eccbcb6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@jeremywiebe I know you mentioned in the discussion that you wanted to see us Fetch the image and draw it to the canvas. As I've looked at this it doesn't seem unreasonable to use the Image HTML element and get the natural size. I've tried to clean up the code a lot (we don't need to worry about IE now that it's Edge and secretly really chrome). Does this feel reasonable? I'm still experimenting but I think the problem is that there was an edge case where we didn't set an image size or set an error. I'm removing that case and trying to figure out how it impacts the editor experience. |
|
Size Change: -177 B (0%) Total Size: 825 kB
ℹ️ View Unchanged
|
|
|
||
| Util.getImageSize(url, (width, height) => { | ||
| if (this._leadingUrl !== url) { | ||
| return; |
There was a problem hiding this comment.
So I think this is the source of the errors. If the previous URL doesn't match the current URL then this function fails, shows no error and gives up. The note about why we were doing this was to prevent weirdness but I'll take correctness with some weirdness over a smooth but deceptively incorrect experience.
I'm figuring out how to get this editor running so that I can interactively play with it to confirm that this is the problem.
| } | ||
|
|
||
| // Handle the error case | ||
| image.onerror = reject; |
There was a problem hiding this comment.
This seems like another place where we'll shore up this image size calculation process. The old getImageSize didn't attach an onerror.
| onUrlChange: async function (url: string | undefined | null, silent) { | ||
| // Check if we've been passed something that looks like a URL | ||
| if (!url) { | ||
| this.setUrl(url, 0, 0, silent); |
There was a problem hiding this comment.
Looking at setUrl, if we get a value that's not a valid URL (even null or undefined), we'll set that value onto the Image.url attribute and the new error handling should handle it? Just want to make sure we aren't burying a new error here.
nixterrimus
force-pushed
the
new/image-size-loading
branch
from
475f9c3
to
32674f9
Compare
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (e250cc0) and published it to npm. You Example: yarn add @khanacademy/perseus@PR618 |
.changeset/heavy-deers-give.md
Outdated
| "@khanacademy/perseus-editor": minor | ||
| --- | ||
|
|
||
| Update image editor so it has a small preview |
There was a problem hiding this comment.
What do you think about also mentioning the bug fix here?
| } catch (error) { | ||
| this.setState({ | ||
| backgroundImageError: | ||
| "There was an error loading the image URL", |
There was a problem hiding this comment.
I wonder if we want to provide the error text in this message? For learner-facing I think we'd want more polish, but the source error might give more insight to the content author why the image couldn't load. What do you think?
Summary:
This fixes an error where images where getting no size information. In addition it also adds preview information to the image editor pane so that content editors get better visibility into what's going on.
I think this gives us two ways to catch image problems before they get out to prod: better code and human intervention.
Issue: https://khanacademy.atlassian.net/browse/LC-821
Test plan: