Fix Cloudinary image uploads via Netlify CMS #271
Conversation
|
Deploy preview for texasjusticeinitiative ready! Built with commit 9f02f98 https://deploy-preview-271--texasjusticeinitiative.netlify.com |
Automatically generated. Merged on Netlify CMS.
Automatically generated. Merged on Netlify CMS.
static/admin/config.yml
Outdated
| @@ -3,7 +3,7 @@ backend: | |||
| # Swap out "git-gateway" with "test-repo" and | |||
| # Navigate to localhost:3333/static/admin/index.html to view Netlify CMS locally. | |||
| name: "git-gateway" | |||
| branch: master | |||
| branch: fix-cloudinary-images | |||
There was a problem hiding this comment.
I will change this branch back to master before I merge this PR, but I'm leaving it as-is right now so that we can check out the CMS in the preview deploy.
|
|
||
| return ( | ||
| <Image publicId={path.basename(url)} cloud_name="texas-justice-initiative" secure="true" alt={alt}> | ||
| <Transformation width={maxWidth} crop="scale" aspect_ratio={aspectRatio} /> |
There was a problem hiding this comment.
I called this prop maxWidth to communicate that it doesn't actually control the width attribute on the compiled <img> tag. Instead, it's used to specify the width of the image we'd like Cloudinary to return, and we want that to be largest width at which we'll display the image.
Does that seem intuitive, or can you think of a better name or way to handle this?
There was a problem hiding this comment.
I think it's fine. We could add a comment in to explain it, but I honestly think it's intuitive enough what we are trying to pass here.
| const { url, alt, maxWidth, aspectRatio } = this.props; | ||
|
|
||
| return ( | ||
| <Image publicId={path.basename(url)} cloud_name="texas-justice-initiative" secure="true" alt={alt}> |
There was a problem hiding this comment.
Node has this neat method for extracting the file name from a URL!
| url: PropTypes.string.isRequired, | ||
| alt: PropTypes.string.isRequired, | ||
| maxWidth: PropTypes.number.isRequired, | ||
| aspectRatio: PropTypes.number, |
There was a problem hiding this comment.
aspectRatio is optional, and it's useful when we want to force the image to be a square like we do for the volunteer images.
nholden
force-pushed
the
fix-cloudinary-images
branch
from
7da4b56
to
6775640
Compare
nholden
changed the title
There was a problem hiding this comment.
Looks fine to me @nholden. I didn't quite understand what the issue was with images not displaying correctly. When I reviewed the original PR they were showing up? Sounds like you and Eva may have run into an issue this week? I'll catch up on it today.
|
|
||
| return ( | ||
| <Image publicId={path.basename(url)} cloud_name="texas-justice-initiative" secure="true" alt={alt}> | ||
| <Transformation width={maxWidth} crop="scale" aspect_ratio={aspectRatio} /> |
There was a problem hiding this comment.
I think it's fine. We could add a comment in to explain it, but I honestly think it's intuitive enough what we are trying to pass here.
|
Thanks for the review, @LayaTaal!
The images were displaying correctly because I had manually updated our What I should have done was updated the config via the CMS instead of doing it manually. It would have been more time consuming, but I would have learned that my assumption about FWIW, all of the URLs in the config in this PR were generated by the CMS. |
We're currently using the
cloudinary-reactlibrary to include images from Cloudinary on the site. That library's<Image>component expects a filename as apublicIdprop. If we pass it a full URL instead, Cloudinary won't perform our desired transformations and we end up downloading unoptimized images.So that we could pass just filenames to
<Image>, I had originally set theoutput_filename_only: trueconfiguration for the Cloudinary media library so that Netlify CMS would store filenames instead of full URLs in our markdown. This ended up causing two issues:output_filename_only: true.output_filename_only: truewasn't even outputting filenames! It output non-existent local paths like this one.Moral of the story:
output_filename_only: trueis bad news. This PR removes that configuration. As a result, we store full Cloudinary URLs in our markdown, and it's our responsibility to grab the filenames from those URLs and pass them along to<Image>.There was already some duplication in the way we inserted Cloudinary images, and this change would introduce more, so I extracted a new
CloudinaryImagecomponent as a wrapper around thecloudinary-reactfunctionality.