-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Repair Cloudinary Select and extend upload functionality v2 #4774
Conversation
…ublic_id to lightbox caption
…video tag for the moment
…verwrite field, UI updates, no multi support yet
…ders option set to false
…et default behavior as error instead of overwrite
…ixed issue where filename was not sanitized first so was not matching existing files of the same name
…s are maintained when parsed and saved in cache storage
…y adding error.detail to Update route Error response
…at caused overwriting of detail if error is an instance of Error
… min after invalid check on max
prefix += `${listPath}/${path}`; | ||
} | ||
|
||
$.get('/keystone/api/cloudinary/autocomplete', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works pretty well, I did see that this line needs updating to this.
${Keystone.adminPath}/api/cloudinary/autocomplete
I would like to see this one get merged into master. Kind regards |
@barbarosso This PR is an updated version of #4633 but still has the same issues in terms of being a monolithic change set that is difficult to review & test. There don't appear to be any documentation or test changes, which seems surprising for 48 commits and 19 files changed. The PR also doesn't mention what testing has been done (for example, browser support). To make this more easily reviewable I would still be inclined to cherrypick and group related fixes, add tests and documentation, and submit several individual PRs. For example, the "Flash messages in admin UI list view" could be a distinct and reviewable changeset that should likely be done first (since the Cloudinary changes make use of this).
I'd also like to see the changes reviewed and merged, but there's currently a lot of prep work required with a PR of this size and scope. Keystone contributors are volunteering their time and also inherit the support and maintenance of code and documentation once changes are merged into master. Some help breaking up the monolith into easily reviewable PRs (including tests & docs) would definitely move this along faster. For some tips on great pull requests, see: https://www.atlassian.com/blog/git/written-unwritten-guide-pull-requests. Regards, |
@barbarosso @stennie Yep, this is something I definitely want to split up and make easier for review, just haven't had any time lately. Any help would be much appreciated (and I'm glad to coordinate if you send me a message)! |
@chasms, I just checked your branch and it looks great! I did find one minor issue though, when selecting from the dropdown it gets cut by the modal instead of overflowing: I think it can be fixed by changing |
Hey @chasms , we are entering maintenance mode for keystone 4 ( see keystonejs/keystone#4913). |
@laurenskling I need to find some time for this. First thing I imagine is to rebase and smoke test, make sure it is still working with the latest. I've lost a lot of context since I last worked on this, and it is a bit monolithic but a lot of pieces are interdependent. Will likely take some effort to split up. I'll keep you posted on my ability to find some time for this. |
Just a thought; but I think it might be the best plan to take this PR as a reference in what to do. Take care of each bullet point, in good order, one by one. By creating new PR's. This is a big thing for me, as one of the new maintainers, to get CloudinaryImage back in shape. I'm pretty sure most users of Keystone, like me, use this |
@laurenskling sounds good. Do you have a demo project with resources all set up for testing the package live with |
I don't have anything like that ready to go at the moment, no. :/ |
Rebase of #4633
Description of changes
Restored select functionality of CloudinaryImageType input field Update dependency @types/keystonejs__adapter-knex to ^6.3.2 keystone#4099
Restored
selectPrefix
option functionality of CloudinaryImageType schema type so that select options are scoped to the correct path in CloudinaryAdded support for additional filetypes in Cloudinary, including video
Updated behavior of CloudinaryImageType with
filenameAsPublicID
option to use the filename and to throw a semantic error when filename already exists as related to discussion here: Docs: link style for headings keystone#925Set default behavior of CloudinaryImageType to use the Cloudinary
use_filename: true
convention - by which the publicID is set to the filename + a short uniqueID on upload to prevent collision - and use retry forwhenExists
Fixed issue where CloudinaryImageType's getExists was checking filename without first sanitizing and removing extension, causing false negatives
Added new
uploadOptions
for Cloudinary so that upload presets, eager transformations, and other cloudinary upload options can be set per field on the schema all in an object that is passed to the upload request (so that all options are accommodated):flow-bin 0.92.0 causes errors keystone#832,
Apply applyIdFieldDefaults before createSystem keystone#4459, createItems throws Error when using Lists Back References keystone#2360, add dependsOn option in field which is same as v4 keystone#2260,
https://cloudinary.com/blog/centralized_control_for_image_upload_image_size_format_thumbnail_generation_tagging_and_more
Added detail error messages for "Field Error" errors in
EditForm
AlertMessages
, e.g. from Cloudinary upload responses whenwhenExists: 'error'
:"Field errors" keystone-storage-adapter-s3#9
Added detail error messages for custom
pre('save'
andpre('validate'
hooks by addingerror.detail
into theres.apiError
call of theUpdate
route:Mongodb sorting issue with mongodb when list has more than 10000 items keystone#3348
Inside of the hooks, errors should be compiled like this to activate this functionality:
Added defensive code to prevent breaking UI when cloudinaryImageSummary attempts to render empty image in table cell
Added textArea character counts on UI per spec in Field validation: Max characters #126
Related issues (if any)
getRequestHandler
methods Add filter and CRUD test for File field type keystone#3433Testing
npm run test-all
ran successfully.