Skip to content
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

Fixes #994. Better handle 413 errors when uploading an image. #1016

Merged
merged 3 commits into from
Apr 27, 2016

Conversation

miketaylr
Copy link
Member

This PR has 3 related changes:

  1. Show a better flash message when the server returns a 413 (image too large)
  2. Fix the z-index issue where the flash message was behind the image preview
  3. Fix bugs in "image too large" validation that meant submit buttons were never re-enabled if you removed or changed the image.

r? @magsout

@miketaylr
Copy link
Member Author

(Redirecting review to Adam, I think @magsout is swamped at work right now -- no issues ❤️ )

r? @adamopenweb

@miketaylr
Copy link
Member Author

OK, now that #1018 is in will fix the merge conflicts.

Mike Taylor added 3 commits April 27, 2016 14:50
Also test that img_too_big isn't true before re-enabling submits.
Also, make sure z-index is higher than the image preview.
@miketaylr
Copy link
Member Author

Alright, rebased and fixed conflicts (and just manually tested again).

when travis is green, can you take a look @adamopenweb?

@@ -284,7 +299,7 @@ function BugForm() {
};

this.addPreviewBackground = function(dataURI) {
if (!_.startsWith(dataURI, 'data:image/png;base64')) {
if (!_.startsWith(dataURI, 'data:image/')) {

This comment was marked as abuse.

@magsout
Copy link
Member

magsout commented Apr 27, 2016

lgtm 👍 r+ ?

sorry for the daly ..

@magsout
Copy link
Member

magsout commented Apr 27, 2016

But you ask @adamopenweb so maybe let him do the review ?

@miketaylr
Copy link
Member Author

No worries about the delay @magsout!

@miketaylr
Copy link
Member Author

I'm gonna go ahead and merge, I think @adamopenweb was waiting on me to fix merge conflict junk before reviewing -- then I can do a deploy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants