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

Added thumbnail to wagtail CMS #625

Merged
merged 1 commit into from
Jun 29, 2016
Merged

Added thumbnail to wagtail CMS #625

merged 1 commit into from
Jun 29, 2016

Conversation

noisecapella
Copy link
Contributor

@noisecapella noisecapella commented Jun 23, 2016

What are the relevant tickets?

Fixes #518

What's this PR do?

Adds a field to the program page which represents the thumbnail shown on the home page for that program.

How should this be manually tested?

Before you upload any images, first make sure the default image looks appropriate. It will be the same filler image as before but with the correct aspect ratio. If you resize the browser window you should see this image get smaller or bigger to fill the box space which is roughly half the main container.

Run database migrations. Then in /cms/ upload an image using the Images link on the left sidebar. Then go into a program page and set the thumbnail image to the image you uploaded. Go to the home page and see the program page with the new thumbnail. Resize the browser to make sure the image stretches and shrinks appropriately. Note that the image must always keep the correct aspect ratio and the height should not change. Anything toward the bottom of the image should be cut off.

Any background context you want to provide?

edx course images are 378x225 so the homepage will probably be redesigned around these smaller boxes at some point soon.

Screenshots (if appropriate)

Full window:
screenshot from 2016-06-23 11-06-12

Resized smaller:
screenshot from 2016-06-23 11-06-38

@bdero bdero temporarily deployed to micromasters-ci-pr-625 June 23, 2016 14:40 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-625 June 23, 2016 14:57 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-625 June 23, 2016 15:24 Inactive
@giocalitri giocalitri self-assigned this Jun 24, 2016
@@ -87,6 +95,7 @@ class ProgramPage(Page):
FieldPanel('title_over_image'),
InlinePanel('courses', label='Program Courses'),
InlinePanel('faqs', label='Frequently Asked Questions'),
FieldPanel('thumbnail_image'),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should go before the courses and faqs, because these lists can be really long.
Maybe put at the beginning right before the link to the external page?

@giocalitri
Copy link
Contributor

I think that resizing the page changes the height of the visible part of the image.
screen shot 2016-06-24 at 11 01 30 am

screen shot 2016-06-24 at 11 01 45 am

@giocalitri
Copy link
Contributor

2 minor comments, but overall good.

@giocalitri
Copy link
Contributor

cc @roberthouse54

@roberthouse54
Copy link
Contributor

Yeah, see if you can fix the sizing of that image. I think the image should just be 100% the width of the box, with no height specified. It is being clipped apparently

@bdero bdero temporarily deployed to micromasters-ci-pr-625 June 24, 2016 15:27 Inactive
@noisecapella
Copy link
Contributor Author

Comments are addressed

@giocalitri
Copy link
Contributor

ok, but now is something like this acceptable?
screen shot 2016-06-24 at 12 11 41 pm

@giocalitri
Copy link
Contributor

in other words: can we leave the users the possibility to mess up with our home page?

@roberthouse54
Copy link
Contributor

I was under the impression that the EDx course thumbnails were all the same size... No?

@noisecapella
Copy link
Contributor Author

The CMS users aren't ordinary users, they should be trusted to upload the right thumbnails

@giocalitri
Copy link
Contributor

@roberthouse54 No idea

@noisecapella Fine with me

@pdpinch ??

@Ferdi
Copy link
Contributor

Ferdi commented Jun 24, 2016

  • These are the "Micromasters thumbnails" not course thumbnails, so we dont have to think edx in our design
  • Is this enabled in our app ? https://wagtail.io/features/image-cropping/ . If so I'd assume user selected the relevant area of the image so we can safely scale/crop at some predefined image sizes.

@noisecapella
Copy link
Contributor Author

@Ferdi Yes it should be. If we aren't using edX thumbnails, how big should we expect our thumbnails to be?

@Ferdi
Copy link
Contributor

Ferdi commented Jun 24, 2016

No idea, @roberthouse54 ?

@roberthouse54
Copy link
Contributor

Sorry I was thinking of the EDx course thumbnails. OK, so the program thumbnails should be the same aspect ration as the EDx course thumbnails. So the program thumbnail can be the same size 378x225. If the image cropping is working, can we set it to crop at that aspect ratio, and then save the image at that size?

@bdero bdero temporarily deployed to micromasters-ci-pr-625 June 24, 2016 20:56 Inactive
@noisecapella
Copy link
Contributor Author

Just made the change. To clarify, here's what's happening:

  • User uploads an image to the CMS images section. They can optionally drag a focus area, but this is not a cropping tool, it just tells wagtail where the center of the image is if something needs to be cropped.
  • Image is output at 378x225 or smaller resolution, keeping aspect ratio. It's using the 'fill' resize rule which means that it will resize the image smaller to fit this resolution, then crop whatever doesn't fit. It uses the focus area to determine where to crop, or else just the center of the image.
  • The image is then displayed in the browser to fill 100% of the box width, then however much height it needs, keeping the aspect ratio the same.

@bdero bdero temporarily deployed to micromasters-ci-pr-625 June 24, 2016 21:15 Inactive
@giocalitri
Copy link
Contributor

this is the output I see: is that what is expected?
screen shot 2016-06-27 at 2 32 28 pm

@noisecapella
Copy link
Contributor Author

Yes, it should have the same aspect ratio as 378x225, cropped to that size, and scaled up to whatever width the box has in the browser

@roberthouse54
Copy link
Contributor

I am still unclear what is happening. We need the images to all have the same aspect ratio. These two images in the mock above do not seem to have the same aspect ratio.

@noisecapella
Copy link
Contributor Author

The image on the right is the default image if no image was picked. I'll crop that image to have the same resolution as the other ones

@bdero bdero temporarily deployed to micromasters-ci-pr-625 June 27, 2016 19:57 Inactive
@noisecapella
Copy link
Contributor Author

noisecapella commented Jun 27, 2016

The default thumbnail should be the same aspect ratio now. Here's a new screenshot:
screenshot from 2016-06-27 15-57-49

The box on the bottom isn't necessarily the same size. Should it we make it the same size?

@giocalitri
Copy link
Contributor

tests are failing

@giocalitri
Copy link
Contributor

I suspect this is expected, but still I would like an opinion
screen shot 2016-06-27 at 4 19 34 pm

@roberthouse54
Copy link
Contributor

Truncate to some reasonable character limit with a "... (View More)" link to the program page.

@giocalitri
Copy link
Contributor

that text comes from the django model and not the CMS: I wonder if the should move it to the CMS...

@bdero bdero temporarily deployed to micromasters-ci-pr-625 June 29, 2016 14:42 Inactive
@bdero bdero temporarily deployed to micromasters-ci-pr-625 June 29, 2016 14:43 Inactive
@noisecapella
Copy link
Contributor Author

@giocalitri Filed #656

@noisecapella
Copy link
Contributor Author

noisecapella commented Jun 29, 2016

@roberthouse54 I'm going to file an issue for the View More link, in the interest of getting this PR merged first
edit #658

@giocalitri
Copy link
Contributor

please rebase and squash. Then 👍

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

Successfully merging this pull request may close these issues.

5 participants