Status

Webmaker
Popcorn Maker
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: brett, Assigned: thecount)

Tracking

Details

(Whiteboard: popbetter)

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Often with images you'll want them to fill the maximum area allowed by the aspect ratio.  Currently this is tedious - you need to move and scale the image to do this.  An option to fill the canvas would speed things up for makers
(Reporter)

Comment 1

4 years ago
Kate had demos in her drag and drop that solved this.  Scott, can you find?
Flags: needinfo?(scott)
Summary: Image plugin needs option to fill screen → Implement crop
(Assignee)

Comment 2

4 years ago
http://k88hudson.github.io/bestdraggable/#images-tab-container

That's it.

We should do that.

We can also do a simple full screen button to the most common usecase of image resize even easier.
Flags: needinfo?(scott)
(Reporter)

Comment 3

4 years ago
yes, do!
(Assignee)

Updated

4 years ago
Assignee: nobody → scott
(Assignee)

Updated

4 years ago
Depends on: 893796
(Assignee)

Comment 4

4 years ago
Created attachment 797655 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/189

Finally!

Been tracking down some growth issues when I converted from using pixels to percentages. Took a bit to get right.
Attachment #797655 - Flags: review?(schranz.m)
Comment on attachment 797655 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/189

Some questions for now. There is a lot of thought process behind this code that is hard to pick up by just reading it.
Attachment #797655 - Flags: review?(schranz.m) → review-
(Assignee)

Comment 6

4 years ago
Comment on attachment 797655 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/189

Updates.
Attachment #797655 - Flags: review- → review?(schranz.m)
Comment on attachment 797655 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/189

I feel the code here is really solid, but would like a follow up ticket filed.

Right now resizing the actual container on the top/left start's moving the actual image around inside it when really it shouldn't do that. There were technical reasons behind why it's doing it right now and i'd rather land the feature itself.

There are however bugs. For example, after double clicking on the container after switching to the flickr slide show... https://dl-web.dropbox.com/spa/qxpeeqhhsjhcqcf/8t26yt5f.png
Attachment #797655 - Flags: review?(schranz.m) → review-
Also, that tooltip shouldn't really be present when it's not a normal image. Should be able to use https://github.com/mozilla/popcorn.webmaker.org/commit/b3fcc5d1c11ccbf37d00dc7c0d49210d6bf29af5 to accomplish hiding it.
Probably going to want to add https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/templates/assets/editors/editorhelper.js#L249-L251 and https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/templates/assets/editors/editorhelper.js#L132-L134 to your code as well to prevent extra handles being generated.
(Assignee)

Comment 10

4 years ago
Comment on attachment 797655 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/189

Updates.
Attachment #797655 - Flags: review- → review?(schranz.m)
Comment on attachment 797655 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/189

Flickr Slideshow is broken. This is thrown to the console when I select it:

GET http://localhost:8888/undefined 404 (Not Found)
Attachment #797655 - Flags: review?(schranz.m) → review-
(Assignee)

Comment 12

4 years ago
Comment on attachment 797655 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/189

Was there anything else?

Anyway, flickr is doing this on master. Looks like a broken link in the first of three images under the mozilla tag.

If you change the tag, it works fine, or if you play anyway, 2 of the 3 mozilla tagged images do load. First one is the dead link.

I think we should file it.
Attachment #797655 - Flags: review- → review?(schranz.m)
Comment on attachment 797655 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/189

R+ with nits in comments and:

public/templates/assets/editors/image/image.js: line 30, col 5, Function declarations should not be placed in blocks. Use a function expression or move the statement to the top of the outer function.
public/templates/assets/editors/image/image.js: line 42, col 11, '$' is not defined.
public/templates/assets/editors/image/image.js: line 43, col 7, '$' is not defined.
public/templates/assets/editors/image/image.js: line 141, col 7, '$' is not defined.
public/templates/assets/editors/image/image.js: line 158, col 7, '$' is not defined.

These are lint errors introduced by this patch. I know it isn't passing lint right now but if we don't introduce more that would be nice.

Also, if you haven't filed a bug for that problem with flickr, can you?
Attachment #797655 - Flags: review?(schranz.m) → review+
(Assignee)

Comment 14

4 years ago
Comment on attachment 797655 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/189

Filed it as bug 915738

I fixed the lint issues and added a comment.

Going to throw it back to you for one last sanity check.
Attachment #797655 - Flags: review+ → review?(schranz.m)
Attachment #797655 - Flags: review?(schranz.m) → review+
(Assignee)

Comment 15

4 years ago
Staged: https://github.com/mozilla/popcorn.webmaker.org/commit/c149b3991a801322c7dc02d32300590476bb7695

Needs verification.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(scott)
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Status: RESOLVED → VERIFIED
Flags: needinfo?(scott)
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.