-
Notifications
You must be signed in to change notification settings - Fork 351
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll hold off to approve as this is still in draft, but I think the approach looks fine.
} | ||
|
||
// Handle the error case | ||
image.onerror = reject; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excited to get this fixed. Thanks for adding the image preview, that's a really nice touch!
.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: