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

Remove thumbnail cropping, and default thumbnail aspect ratio to 16:9 #1369

Merged
merged 8 commits into from
May 21, 2019

Conversation

kollivier
Copy link
Contributor

Description

Removes code to crop images to have a square aspect ratio. This would sometimes create oddly cropped thumbnails, and Kolibri is capable of displaying 16:9 ratio thumbnails, so we remove the cropping to ensure the thumbnails look the same in Studio and Kolibri.

Note that this is basically removing code, so existing test coverage should ensure the code is still working.

Issue Addressed (if applicable)

Addresses #1266

Steps to Test

  • Assign a wide thumbnail to a node and make sure it is not cropped after publishing.
  • Assign a square / close to square thumbnail and ensure it displays properly after publishing

Does this introduce any tech-debt items?

There are still a couple places that assume square thumbnails in the UI, but addressing this can probably happen in a future release.

Checklist

Delete any items that don't apply

  • Is the code clean and well-commented?

Reviewers

If you are looking to assign a reviewer, here are some options:

  • Jordan jayoshih (full stack)
  • Aron aronasorman (back end, devops)
  • Micah micahscopes (full stack)
  • Kevin kollivier (back end)
  • Ivan ivanistheone (Ricecooker)
  • Richard rtibbles (full stack, Kolibri)
  • Radina @radinamatic (documentation)

@kollivier kollivier requested a review from jayoshih May 15, 2019 23:36
@kollivier kollivier added the qa-ready Create a demo server for this pull request label May 15, 2019
Copy link
Contributor

@jayoshih jayoshih 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, but I do want to be a little careful merging this in right away without addressing a few other concerns:

  1. When I run Studio locally, I'm not getting the 16:9 channel aspect ratio, so we might need to change that too
  2. We haven't added the tools for handling non 16:9 thumbnails like filling in the rest of the space of the image
  3. We need to update the front-end croppie wrapper to have this ratio as well

I can tackle these, but we'll probably want to merge these changes in with this PR

@kollivier
Copy link
Contributor Author

  1. When you say you're not getting the 16:9 aspect ratio, what do you mean? It seemed to work in my tests. There are a couple places in the UI where the box still has square dimensions for channels, but the actual thumbnail is 16:9 when I check it in Kolibri.

  2. For non-16:9 thumbnails, we will just have empty space to the left and right. We don't need to enforce a particular image pixel size, just that it displays centered in a 16:9 thumbnail display. This approach should work for any reasonable thumbnail aspect ratio. (If someone tries to use a panorama as a thumbnail, the solution would be to edit it manually to be something reasonable first.)

  3. I just checked and Croppie has 16:9 for content node thumbnails, so we just need to change the aspect ratio for channel thumbnails. That is a one line change, so I can take care of that and push an update!

@jayoshih
Copy link
Contributor

When you say you're not getting the 16:9 aspect ratio, what do you mean?

I'm not able to replicate #1336 locally, so I'm not sure if merging things in will make it square again

For non-16:9 thumbnails, we will just have empty space to the left and right. We don't need to enforce a particular image pixel size, just that it displays centered in a 16:9 thumbnail display.

That makes sense, although I feel like the thumbnails might feel a little 'ill-fitted' for the frame (echoing some of the thoughts here). It would be nice to have some way to fill up the rest of the space, but perhaps for another time

That is a one line change, so I can take care of that and push an update!

Thanks! There are a few other places you'll need to update as well:

Along with that, you'll need to update the kolibri_placeholder.png to have the correct aspect ratio. Here's one you can use

@kollivier
Copy link
Contributor Author

Thanks for the detailed information and help! I just pushed fixes for the remaining thumbnail display locations. I'm fine with taking more time with this and not pushing it in the next release, although I would say that we look at this change from the perspective of does it make anything worse than it is today rather than does it solve all the issues.

For example, it's true that now some thumbnails won't look great as we move from square to 16:9 for channels, but they won't look any worse in Kolibri than they do now, and them looking less good in Studio will hopefully serve as a motivator to improve the thumbnail to match our new guidelines. 😄Love to hear your thoughts on how things look now!

@jayoshih
Copy link
Contributor

Alright, just a few more things I noticed as I was going through the demo server:

  • The channel settings modal inside the channel edit page uses the square thumbnail ratio (also, need to make the cropping mode width smaller)
  • When I upload a square thumbnail, I can't zoom out when I crop it past the boundary. Could you set the enforceBoundary prop to be false? (docs)
  • Quick fix for non 16:9 thumbnails- could you set the .channel-pic class surrounding it to have a white background?

As for this push as a whole, the interface looks great to me! My main hesitation is primarily from the repercussions on the content sources. 16:9 isn't really a common aspect ratio for brand-related assets. However, this seems to be okay given that thumbnails of any size should still be displayed

@jayoshih
Copy link
Contributor

Also resolves #1286

@jayoshih
Copy link
Contributor

jayoshih commented May 20, 2019

A few more things before we can merge:

  • Channel settings modal needs to be updated with the 16:9 ratio and styling needs to better fit the ratio (no overlap between thumbnail and metadata)
    image

  • Clipboard icons need a white background and maybe need to be bigger
    image

  • Channel details source tab should have 16:9 thumbnails
    image

@kollivier
Copy link
Contributor Author

Okay, just pushed fixes for the above issues!

@jayoshih
Copy link
Contributor

Awesome! Merging now

@jayoshih jayoshih merged commit 96e1b5e into learningequality:develop May 21, 2019
@jayoshih jayoshih added changelog Needs to be added to the changelog (remove once added) and removed qa-ready Create a demo server for this pull request changelog Needs to be added to the changelog (remove once added) labels May 21, 2019
@kollivier kollivier deleted the stop-the-crop branch June 10, 2019 16:55
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