Skip to content

Conversation

@tzkmx
Copy link

@tzkmx tzkmx commented Aug 11, 2021

  • The disk is taken into account to locate the file onto which was uploaded.
  • The saved_path is sent to the delete action, and only in success response the image is removed from the client array.

this.images.splice(index, 1);
this.value = JSON.stringify(this.images);
})
.catch(console.warn)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not console.error ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it is not an error in the frontend, and indeed this is not the best way to handle the error, actually it should show a toast or mark the file in some way to let the user know that maybe communication with the backend was temporarily not possible (timed-out), the file could not be found, the path was wrong, etc.

return "success";
return $deleted === true
? 'success'
: response('not deleted', 404);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be better like that ?

return $deleted === true
            ? response()->noContent()
            : response('Files not found.', 404);

success as a response is not restfull and useless.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this should be really a RESTful controller, but this works so I don't know about no use.
Presently, there is not even a real entity being represented here, but I've been thinking that it could be a way to handle #13 having a handler of Preloaded images but not finally attached to the resource, like abandoned editing.

I haven't time to start a new package handling all these issues at once, and was expecting the original writer could attend to the PR's but I'll keep in mind your observation and think the handling of the images in a more restful way.

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.

2 participants