Skip to content

Fixes #722 - Handle two sizes images #1266

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 6, 2017
Merged

Fixes #722 - Handle two sizes images #1266

merged 5 commits into from
Jan 6, 2017

Conversation

karlcow
Copy link
Member

@karlcow karlcow commented Jan 5, 2017

This modifies the python upload code and the JavaScript libraries handling the markup.
Note that for the JS, the previous code is using two different methods for inserting the code at the right place even if they do exactly the same thing. This should be abstracted.

r? @miketaylr

@karlcow karlcow requested a review from miketaylr January 5, 2017 12:53
@karlcow karlcow mentioned this pull request Jan 5, 2017
Closed
miketaylr
miketaylr requested changes Jan 5, 2017
View reviewed changes
Copy link
Member

@miketaylr miketaylr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Would it be possible to add a test or two to test_uploads.py? I'm not actually sure what that would look like, since the tests are a bit of a hack, just testing responses. (I won't block on tests, but it would be nice!)

❤️

webcompat/static/js/lib/bugform.js
this.addImageURL = function(url) {
var file_ext = url.split('.').pop();
var thumb_url = url.replace('.' + file_ext, '-thumb.' + file_ext);
var imageURL = ['[![Screenshot Description](', thumb_url, ')](', url, ')'].join('');

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

webcompat/static/js/lib/bugform.js
var imageURL = ['![Screenshot Description](', url, ')'].join('');
this.addImageURL = function(url) {
var file_ext = url.split('.').pop();
var thumb_url = url.replace('.' + file_ext, '-thumb.' + file_ext);

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

webcompat/static/js/lib/issues.js
var compiledImageURL = imageURL({url: response.url});
var img_url = response.url;
var file_ext = img_url.split('.').pop();
var thumb_url = img_url.replace('.' + file_ext, '-thumb.' + file_ext);

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@karlcow
Copy link
Member Author

karlcow commented Jan 6, 2017

ok addressed all the comments. And also pushed up a bit the size of the image and changed the algorithm for compression, so screenshots of texts are usable on desktop.

NEAREST at the top, BILINEAR at the bottom.
capture d ecran 2017-01-06 a 09 39 03

@karlcow
Copy link
Member Author

karlcow commented Jan 6, 2017

Thanks @miketaylr for the cool review. :)

@miketaylr
Copy link
Member

Looks good! r+

(Can you file an issue for adding some thumbnail tests?)

@miketaylr miketaylr merged commit f69f071 into webcompat:master Jan 6, 2017
@karlcow karlcow mentioned this pull request Jan 6, 2017
Closed
@karlcow
Copy link
Member Author

karlcow commented Jan 6, 2017

ahah. forgotten about that. Done with #1267

❤️

This was referenced Jan 6, 2017
Closed
Merged
@miketaylr
Copy link
Member

Let's revert this for now, sorry @karlcow!

Another thing to fix when you figure out why the bug form uploads are broken: if you click "Remove image upload", it should remove the link in description.

See https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/bugform.js#L383-L385

@miketaylr miketaylr mentioned this pull request Jan 6, 2017
Closed
3 tasks
@karlcow karlcow deleted the 722/3 branch March 30, 2017 05:36
@karlcow karlcow restored the 722/3 branch March 30, 2017 05:37
@karlcow karlcow deleted the 722/3 branch August 24, 2017 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miketaylr miketaylr miketaylr requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@karlcow @miketaylr