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

Repair Cloudinary Select and extend upload functionality v2 #4774

Closed
wants to merge 48 commits into from
Closed

Repair Cloudinary Select and extend upload functionality v2 #4774

wants to merge 48 commits into from

Conversation

chasms
Copy link

@chasms chasms commented Aug 21, 2018

Rebase of #4633

Description of changes

Post.schema.pre('validate', async function (next) {
    const error = []

    // on validations, push into the errors array
    // with an object with 'fieldLabel' and 'message' fields
    if (!this.title) {
        errors.push({ fieldLabel: 'title', message: 'No title set' });
    }

    // at end of validation, call next
    // with the compiled errors as the detail
    if (errors.length) {
        const error = new Error('field errors');
        error.detail = errors;
        next(error);
    } else {
        next();
    }
});
  • 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)

Testing

  • Please confirm npm run test-all ran successfully.

danielmahon and others added 30 commits August 20, 2018 17:55
…verwrite field, UI updates, no multi support yet
…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
prefix += `${listPath}/${path}`;
}

$.get('/keystone/api/cloudinary/autocomplete', {
Copy link

@udox-cameron udox-cameron Aug 22, 2018

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

@barbarosso
Copy link

I would like to see this one get merged into master.
Are there any loose ends here or can we help somewhere to get this one released?

Kind regards
Pascal

@stennie
Copy link
Contributor

stennie commented Sep 26, 2018

@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 would like to see this one get merged into master.

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,
Stennie

@chasms
Copy link
Author

chasms commented Sep 26, 2018

@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)!

@barbarosso
Copy link

@chasms @stennie I think i can help some hours next week, but if some preperation could be done to make it easier to help out.

@RecuencoJones
Copy link
Contributor

@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:

image

I think it can be fixed by changing overflow-y: initial in the modal element:

image

@laurenskling
Copy link
Contributor

Hey @chasms , we are entering maintenance mode for keystone 4 ( see keystonejs/keystone#4913).
I would love to get some of your work on Cloudinary into Keystone. Would you be open to helping us fix the issues you've raised here, but as seperate small PR's? This would make it more controllable and testable. We can work on it together to create a step by step upgrade plan to get your fixes into keystone.

@chasms
Copy link
Author

chasms commented May 15, 2019

@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.

@laurenskling
Copy link
Contributor

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.
There hasn't changed much in the codebase since this PR. I can pretty much safely say: nothing in CloudinaryImage.

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 type alot. Would be great to work together to get stuff fixed. You've got my promise I will put in the effort to help you.

@chasms
Copy link
Author

chasms commented May 15, 2019

@laurenskling sounds good. Do you have a demo project with resources all set up for testing the package live with link? I'll need to get a dev setup going again.

@laurenskling
Copy link
Contributor

I don't have anything like that ready to go at the moment, no. :/

@ghost ghost closed this by deleting the head repository Jan 22, 2024
This pull request was closed.
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.

8 participants