added onSuccessAction prop to Upload Widget #487

JoshuaRotimi · 2024-07-04T09:03:53Z

Description

This PR adds an onSuccessAction prop to the CloudUploadWidget component.

The feature makes a few changes to the CloudUploadWidget and CloudUploadButton components by adding a new onSuccessAction prop where a server action can be passed as a function to be called on the success of an image upload.

Issue Ticket Number

Fixes #486

Fixes: #486

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Fix or improve the documentation
[ . ] This change requires a documentation update

vercel · 2024-07-04T09:03:56Z

@JoshuaRotimi is attempting to deploy a commit to the Cloudinary DevX Team on Vercel.

A member of the Team first needs to authorize it.

colbyfayock · 2024-07-04T22:29:33Z

hey @JoshuaRotimi thanks for putting this together. have a few requests:

can you add a little more context and usage details to the PR for historical and release notes?
i tried using this with CldUploadButton and realized that it wasn't set up to pass the props through, can you set this up so that CldUploadButton can use it as well?

the FormData got me thinking, is ther eanything wrong wiht passing JSON to a Server Action? i tested passing the upload result directly to the onSuccessAction and it seemed to "just work", that would be an easier way to interface with it, though my original assumption was FormData was the intended format for Server Actions
can you add an example of this working inside of src/tests/nextjs-app so that it runs and can easily be tested?
i think part of my hope for this was that we could automate the action creation for each callback type. in the code for intsance, the way that the callback function currently is determined is by:

const widgetEvent = WIDGET_EVENTS[uploadResult.event] as keyof typeof props;

if ( typeof widgetEvent === 'string' && typeof props[widgetEvent] === 'function' ) {
  const callback = props[widgetEvent] as CldUploadEventCallback;
  callback(uploadResult, {
    widget: widget.current,
    ...instanceMethods
  });
}

it's not unreasonable to think that we could add onto that by doing osmething like:

const widgetEventAction = typeof widgetEvent === 'string' && `${widgetEvent}Action`;

if ( widgetEventAction && typeof props[widgetEventAction] === 'function' ) {
  const action = props[widgetEventAction] as CldUploadEventAction;
  onSuccessAction(uploadResult);
}

wdyt?

JoshuaRotimi · 2024-07-05T11:14:53Z

Hello @colbyfayock

Okay, I will add more information about the feature to the PR.

About the JSON/FormData, I think we could switch to JSON, it makes the code more readable without the Object.entries loop, I thought you may have had a definite reason why FormData was the best option.

If we want to automate the action creation the way you stated, we would have to add types for all the other instances like onDisplayChangedAction, onUploadAddedAction and the rest of the CloudWidgetProps actions. is that what we want to do?

Okay, I can add the same functionality to the CloudUploadButton once we have it working correctly for the CloudUploadWidget.

colbyfayock · 2024-07-23T11:39:07Z

The reason was really just because ive only ever really seen FormData examples, so i was thinking it was the standard, but looking into it more it appears that you can pass anything, and didnt see any objections when i asked around about it on twitter (tho not many responses)

i think adding the types for each one makes sense, we're already doing that for the standard ones, wish there was a way to automate that as well. i think it would make the implementation more complete and more useful for people

thanks Josh! and apologies for the delay, currently traveling

JoshuaRotimi · 2024-07-25T12:46:41Z

Yeah I figured you've been busy as I also did not receive last week's newsletter in my email haha. Hope your trip is going good?

Okay that sounds great. I've made some updates to the code as regards to what we spoke about. Please let me know if everything looks great and then I can implement the same for the CloudUploadButton component. Thanks

colbyfayock · 2024-07-25T16:15:24Z

next-cloudinary/src/components/CldUploadWidget/CldUploadWidget.tsx

@@ -57,6 +58,8 @@ const CldUploadWidget = ({

  const signed = !!signatureEndpoint;

+  const { onSuccessAction } = props;


dont think this is needed anymore

colbyfayock · 2024-07-25T16:16:55Z

next-cloudinary/src/components/CldUploadWidget/CldUploadWidget.tsx

+
+        if ( widgetEventAction && typeof props[widgetEventAction] === 'function' ) {
+          const action = props[widgetEventAction] as CldUploadEventAction;
+          uploadResult.event === 'success' && action(uploadResult);


i think we can remove the 'success' check here right? as long as the function exists and its under the watched events, which at this point i believe both should be true, we should be good, right?

colbyfayock · 2024-07-25T16:20:52Z

next-cloudinary/src/components/CldUploadWidget/CldUploadWidget.types.ts

@@ -34,6 +34,20 @@ export interface CldUploadWidgetProps {
  options?: CloudinaryUploadWidgetOptions;
  signatureEndpoint?: URL | RequestInfo;
  uploadPreset?: string;
+  onSuccessAction?: CldUploadEventAction;
+  onUploadAction?: CldUploadEventAction;


the onUpload function is deprecated and will be removed in a later version (likely a major?), so thinking we probably shouldnt add this as an option. while it will technically work if someone tries because the actions are automatically created, we don't want to document or promote its use as a valid option only for it to be pulled out soon

colbyfayock · 2024-07-25T18:08:11Z

Looks good! Added a few comments. Hopefully a lot will transfer through the props for the Button. And when you get a chance if we can make sure to add it to the documentation 🙏 thank you! -- *colbyfayock.com <http://colbyfayock.com>* Twitter <http://twitter.com/colbyfayock> Youtube <https://youtube.com/colbyfayock> Github <http://github.com/colbyfayock> Email Newsletter <https://www.colbyfayock.com/newsletter/>

…

On Thu, Jul 25, 2024 at 2:47 PM Joshua Olorunnipa ***@***.***> wrote: Yeah I figured you've been busy as I also did not receive last week's newsletter in my email haha. Hope your trip is going good? Okay that sounds great. I've made some updates to the code as regards to what we spoke about. Please let me know if everything looks great and then I can implement the same for the CloudUploadButton component. Thanks — Reply to this email directly, view it on GitHub <#487 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH7GGW3JW4KKBXMM6WWU4DZODXUNAVCNFSM6AAAAABKLDS5ASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJQGIZTQMZSGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

JoshuaRotimi · 2024-07-26T13:23:47Z

Okay sounds great. I'll make the changes and push again. For the docs update, do we want to add all the new methods like onSuccessAction, onDisplayChangedAction to the events part of the docs? For the other functions, clicking the "More Info" link takes you to the Cloudinary API for the function. What do we want to display for these methods?

colbyfayock · 2024-07-26T13:48:38Z

yeah i think under Events makes sense, and that could either be in the same group as Functions or a separate Actions, i dont have a strong opinion there, but maybe a separate Actions section under Events makes sense

im thinking about the differentiator between Actions and Functions here. originally the biggest differentiator was the FormData, but now we're passing the same javascript object as the first argument, but not passing the 2nd argument of the options. we can't pass that 2nd argument because it has javascript instances in there, but mainly, its the same except fro that second argument

any thoughts any anything else we can use to differentiate the two aside from that? for instance, if someone is working clientside, why would they want to use an action vs a function? should someone always use a Function unless they are using Actions, which then its the same but limited functionality? (no options argument)

mostly thinking out loud, thats probably fine, but wondering if we have an opportunity to provide any additional useful information or capabilities as part of the Actions workflow

colbyfayock · 2024-07-26T13:49:35Z

as far as the More Info link, i think we can link to the same spot, as its primarily at this time meant to link to the information about the event that triggers it

colbyfayock · 2024-07-26T13:51:24Z

at a minimum, at the top of the Events section, we can simply state the difference at this time is that the Action doesn't pass in the options argument that includes the widget and instance methods to allow it to function as an Action

JoshuaRotimi · 2024-07-26T14:41:06Z

Yes the difference is in the arguments from both functions. Okay that makes sense. I will add that to the docs.

One quick question, can we add a function like this to avoid repetitiveness in the CloudUploadButton component or we should just follow the current pattern.

  const actionProps = Object.keys(props)
      .filter(key => key.endsWith('Action'))
      .reduce((result, key) => {
        //@ts-ignore
        result[key] = props[key];
        return result;
      }, {});

and then in the return statement, we can just spread the actionProps object.

colbyfayock · 2024-07-26T14:51:52Z

honestly i can't think of a good reason why we don't pass in the entire props object and utilize the UploadWidget and the types to maintain the props that are useful

it's no different than creating a <Button where you spread the rest of the props on a <button {...props to allow someone to configure it as theyd like, so likely you can remove all of the redundantly defined props in that component, do you agree?

JoshuaRotimi · 2024-07-26T15:07:54Z

Okay that's fair. Maybe we can create that as a separate issue? Refactoring now may cause some typescript problems.

colbyfayock · 2024-07-26T15:26:50Z

Yup np colbyfayock.com

…

On Fri, Jul 26, 2024 at 11:08 AM Joshua Olorunnipa ***@***.***> wrote: Okay that's fair. Maybe we can create that as a separate issue? Refactoring now may cause some typescript problems. — Reply to this email directly, view it on GitHub <#487 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH7GGT65D46I42VX46LS2TZOJQ6BAVCNFSM6AAAAABKLDS5ASVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJSHE3DIMZVGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

JoshuaRotimi · 2024-07-29T10:57:40Z

Made the changes and pushed the code.

vercel · 2024-07-29T14:41:15Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
next-cloudinary	✅ Ready (Inspect)	Visit Preview	Jul 30, 2024 0:54am

colbyfayock · 2024-07-29T15:21:15Z

next-cloudinary/src/components/CldUploadButton/CldUploadButton.tsx

@@ -33,13 +33,21 @@ const CldUploadButton = ({
  ...props
 }: CldUploadButtonProps) => {

+  const actionProps = Object.keys(props)


i think we can just pass through all of the props at this point and avoid filtering them down here

this will allow the Button to always inherit any available options on the UploadWidget without having to add it in both places, unless you're doing this because of the TS issue you mentioned before

colbyfayock · 2024-07-29T15:34:44Z

just noticed an issue when testing htis out locally.

it looks like as it currently stands, the rest of the ...props on the CldUploadButton are being passed through to the <button itself

Because the action props are not being destructured from that props object, the event handler is getting applied to the <button and creating a warning

this makes me unsure of whether we should pass through the props as i mentioned before directly to the uploadWidget or not. i lean towards yes and having a different mechanism for passing through the <button props, but this would be a breaking change so we should save that as another Issue that gets grouped in with the next major release #441

so for now, i believe we may need to, unfortunately, destructure all of the Action props similar to what we're doing with the callback function props, unless you have a different idea

that said, i tested it out and its working, so hooray! 🙌

JoshuaRotimi · 2024-07-30T09:38:43Z

just noticed an issue when testing htis out locally.

it looks like as it currently stands, the rest of the ...props on the CldUploadButton are being passed through to the <button itself

Because the action props are not being destructured from that props object, the event handler is getting applied to the <button and creating a warning
this makes me unsure of whether we should pass through the `props` as i mentioned before directly to the `uploadWidget` or not. i lean towards yes and having a different mechanism for passing through the `so for now, i believe we may need to, unfortunately, destructure all of the Action props similar to what we're doing with the callback function props, unless you have a different idea

that said, i tested it out and its working, so hooray! 🙌

Ah I see what you mean, the reason we have that warning is because the button reads only native HTML button attributes like onKeyDown, onPress and the likes. We don't see that error because the ...props object is currently empty as we are spreading everything out on the CldUploadButton. So yes you are right, that may be the best approach for now. I was only using the actionProps function to try to reduce repetition. I will do that now and make another push.

colbyfayock · 2024-07-30T12:59:59Z

this is working great now!! thanks @JoshuaRotimi for the PR and your patience as we worked through it

# [6.7.0](v6.6.2...v6.7.0) (2024-07-30) ### Features * Action Props for CldUploadWidget ([#487](#487)) ([ea29cf3](ea29cf3)), closes [#486](#486)

github-actions · 2024-07-30T13:07:06Z

🎉 This PR is included in version 6.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

JoshuaRotimi · 2024-07-30T16:44:05Z

Awesome. Always happy to contribute!

added onSuccessAction prop to Upload Widget

8283942

fixed onSuccessAction function

3f3f2e4

colbyfayock reviewed Jul 25, 2024

View reviewed changes

added same functionality to CldUploadButton

9d80aff

vercel bot deployed to Preview July 29, 2024 14:42 View deployment

docs

3aa13fc

colbyfayock reviewed Jul 29, 2024

View reviewed changes

type order

a434d39

vercel bot deployed to Preview July 29, 2024 15:24 View deployment

fix button warning

8e8349f

vercel bot deployed to Preview July 30, 2024 12:54 View deployment

colbyfayock merged commit ea29cf3 into cloudinary-community:main Jul 30, 2024
4 checks passed

github-actions bot pushed a commit that referenced this pull request Jul 30, 2024

chore(release): 6.7.0 [skip ci]

838405b

# [6.7.0](v6.6.2...v6.7.0) (2024-07-30) ### Features * Action Props for CldUploadWidget ([#487](#487)) ([ea29cf3](ea29cf3)), closes [#486](#486)

github-actions bot added the released label Jul 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added onSuccessAction prop to Upload Widget #487

added onSuccessAction prop to Upload Widget #487

JoshuaRotimi commented Jul 4, 2024 •

edited by colbyfayock

Loading

vercel bot commented Jul 4, 2024

colbyfayock commented Jul 4, 2024 •

edited

Loading

JoshuaRotimi commented Jul 5, 2024

colbyfayock commented Jul 23, 2024

JoshuaRotimi commented Jul 25, 2024

colbyfayock Jul 25, 2024

colbyfayock Jul 25, 2024

colbyfayock Jul 25, 2024

colbyfayock commented Jul 25, 2024 via email

JoshuaRotimi commented Jul 26, 2024

colbyfayock commented Jul 26, 2024

colbyfayock commented Jul 26, 2024

colbyfayock commented Jul 26, 2024

JoshuaRotimi commented Jul 26, 2024

colbyfayock commented Jul 26, 2024

JoshuaRotimi commented Jul 26, 2024

colbyfayock commented Jul 26, 2024 via email

JoshuaRotimi commented Jul 29, 2024

vercel bot commented Jul 29, 2024 •

edited

Loading

colbyfayock Jul 29, 2024

colbyfayock commented Jul 29, 2024 •

edited

Loading

JoshuaRotimi commented Jul 30, 2024

colbyfayock commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

JoshuaRotimi commented Jul 30, 2024

		@@ -57,6 +58,8 @@ const CldUploadWidget = ({

		const signed = !!signatureEndpoint;

		const { onSuccessAction } = props;

added onSuccessAction prop to Upload Widget #487

added onSuccessAction prop to Upload Widget #487

Conversation

JoshuaRotimi commented Jul 4, 2024 • edited by colbyfayock Loading

Description

Issue Ticket Number

Type of change

vercel bot commented Jul 4, 2024

colbyfayock commented Jul 4, 2024 • edited Loading

JoshuaRotimi commented Jul 5, 2024

colbyfayock commented Jul 23, 2024

JoshuaRotimi commented Jul 25, 2024

colbyfayock Jul 25, 2024

Choose a reason for hiding this comment

colbyfayock Jul 25, 2024

Choose a reason for hiding this comment

colbyfayock Jul 25, 2024

Choose a reason for hiding this comment

colbyfayock commented Jul 25, 2024 via email

JoshuaRotimi commented Jul 26, 2024

colbyfayock commented Jul 26, 2024

colbyfayock commented Jul 26, 2024

colbyfayock commented Jul 26, 2024

JoshuaRotimi commented Jul 26, 2024

colbyfayock commented Jul 26, 2024

JoshuaRotimi commented Jul 26, 2024

colbyfayock commented Jul 26, 2024 via email

JoshuaRotimi commented Jul 29, 2024

vercel bot commented Jul 29, 2024 • edited Loading

colbyfayock Jul 29, 2024

Choose a reason for hiding this comment

colbyfayock commented Jul 29, 2024 • edited Loading

JoshuaRotimi commented Jul 30, 2024

colbyfayock commented Jul 30, 2024

github-actions bot commented Jul 30, 2024

JoshuaRotimi commented Jul 30, 2024

JoshuaRotimi commented Jul 4, 2024 •

edited by colbyfayock

Loading

colbyfayock commented Jul 4, 2024 •

edited

Loading

vercel bot commented Jul 29, 2024 •

edited

Loading

colbyfayock commented Jul 29, 2024 •

edited

Loading