Closed Bug 895252 Opened 11 years ago Closed 11 years ago

Implement crop

Categories

(Webmaker Graveyard :: Popcorn Maker, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: brett, Assigned: thecount)

References

Details

(Whiteboard: popbetter)

Attachments

(1 file)

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
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
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)
yes, do!
Assignee: nobody → scott
Depends on: 893796
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-
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.
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-
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+
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+
Staged: https://github.com/mozilla/popcorn.webmaker.org/commit/c149b3991a801322c7dc02d32300590476bb7695

Needs verification.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: needinfo?(scott)
Resolution: --- → FIXED
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.

Attachment

General

Created:
Updated:
Size: