WIP: Peter fogg/course image upload #700
Conversation
|
Can't wait to try out the feature. Thanks! |
| validate: function(attrs, options) { | ||
| if(attrs.selectedFile && attrs.selectedFile.type !== this.attributes.mimeType) { | ||
| return { | ||
| message: "Only " + this.attributes.fileType + " files can be uploaded. Please select a file ending in ." + this.attributes.fileType.toLowerCase() + " to upload.", |
There was a problem hiding this comment.
I hadn't counted on error messages with variable interpolations -- and I really should have thought about it. :( To properly internationalize this, we're going to need to do something like _.template(gettext("Only {fileType} files can be uploaded. Please select a file ending in {fileTypeLower} to upload."), {fileType: this.attributes.fileType, fileTypeLower: this.attributes.fileType.toLowerCase()}). Which is going to need restructuring the templates to expect internationalized strings, so it doesn't try to pass them through gettext twice. Ugh.
If you need to get this in quick, I'm fine with this approach for now, but the better way to do this would be to internationalize here. Sorry for my poor design decision when I made this.
|
Needs a rebase, and a few comments above. In general though, it's looking really good. |
|
All the changes look good to me 👍 . Are you going to allow PNGs as well? |
|
@cahrens Added PNG support for course images, as well as support for multiple file types in the uploader. |
| event.preventDefault(); | ||
| var upload = new CMS.Models.FileUpload({ | ||
| title: gettext("Upload your course image."), | ||
| message: gettext("Files must be in JPG format."), |
There was a problem hiding this comment.
or png
Also make how we talk about the extensions consistent (JPG vs .jpg). Or as consistent as you can (this is a nit).
|
Make sure BDD spec describes which file types are/are not supported. |
Previously the file upload dialog was PDF- and textbook-specific. The changes are adding parameters to the FileUpload model for the file type, and adding an onSuccess callback to the UploadDialog view. Also moved upload-specific SASS into its own file.
Authors can upload an image (or choose an existing one) from the settings page, using the in-context uploader from PDF textbooks. Includes tests for backwards compatibility with XML courses -- they used a magic filename (images/course_image.jpg) which is mapped to a location in the Mongo contentstore. Still needs some UX work, though the backend plumbing is there.
- Internationalize upload errors. - Move upload tests into their own files. - Refactor upload dialog acceptance tests.
Also change the file uploader to accept multiple file types.
|
Visually looked out the changes (did not check out branch), and they look good to me. Peter will investigate why the job failed. @talbs is going to check out the branch and give it a list whirl before the final OK to merge. |
| @@ -0,0 +1,209 @@ | |||
| // studio - views - uploads | |||
There was a problem hiding this comment.
A few thoughts:
- Since this is something shared across views ideally (within PDF textbooks, Files & Uploads later, and now Settings), this should really be within the sass/elements/ directory.
- Also, in Sass files we use a 2-space tab indent for code (versus the 4 within JS).
- I'd prefer if the body class used to call this functionality was named "feature-upload" (we don't have a lot of body classes focused on features, but I can see this convention being used in the future).
Mind moving this and converting the tabs?
|
@peter-fogg, thanks for the additional work. I created a new course to test out the state of when a course didn't have an image set (there's some copy/markup in the settings.html template to try to handle this) and when I checked out the state of the image on the settings page, I get a broken image (bad path/non-existent file) rather than the "Your course currently does not have an image..." copy/state (see screengrab below). Is there something else that needs to be set up in the logic of the template or something I missed when setting up the copy? |
|
@talbs I'll make those changes. The issue with the placeholder is that the default for course image is |
|
@peter-fogg we don't have a suitable default image, and I can't think of a good one to represent all of the various subject matter/purposes behind courses authored. Unless anyone objects, I'd say lets not force a default image for now. |
|
@talbs Sounds good. Is this good to merge, then? |
|
👍 Sadly, this may be one of the last times I say this to you, "looks good, bring it to Mergetown" |
|
😦 Sad times in merge city. |
WIP: Peter fogg/course image upload
Reset CapaProblem when state HTML is corrupt
YONK-318: Update organizations list API to return number of courses
…style Fix positioning of Course Talk widget
…ter-courses-by-user-lang fix filter courses by user lang
@cahrens This should be mostly finished on the dev side. @singingwolfboy, I've made a few small changes to the file upload dialog and moved some code around to support that. Could you take a quick look at e8f40bc to ensure that everything is still OK?