Add FeedbackButton to DeleteDialog of CrudContextMenu

[jomunker]

---

This was done to give the user feedback if something is loading and/ or goes wrong. Additionally, the FeedbackButton was reworked to use Promises for state updates, which simplifies the usage. This was done after a discussion with @johnnyomair.

PR Checklist

• Verify if the change requires a changeset. See CONTRIBUTING.md
• Link to the respective task if one exists: COM-754
• Provide screenshots/screencasts if the change contains visual changes

Screenshots/screencasts

Screen.Recording.2024-07-01.at.17.03.09.mov

[johnnyomair]

@nsams @thomasdax98 what do you think about using the promise states (pending, resolved, rejected) to handle the display states of the button instead of loading, hasErrors etc. props? I think this is easier to understand and less error-prone as the state transitions are pretty clear. Could there be use cases where I wouldn't want to have this?

[jomunker]

I added a flag called feedbackStateControl to differentiate between the manual handling via props and the automatic handling via a returned promise in the onClick function. This seemed like the most descriptive way of doing it. I discussed this with @jamesricky

[johnnyomair]

It should be possible to make this a non-breaking change targeting main. Also, should we move the Promise detection into a separate PR and discussion?

[johnnyomair]

Couldn't this be inferred based on the passed onClick handler? Is it possible to detect if a function is async or not?

[jomunker]

There are ways but it's not straightforward and probably also not reliable. Additionally, it is way more descriptive with the flag. So users will actually understand whats going on.

[johnnyomair]

@nsams @thomasdax98 what do you think?

[nsams]

I would prefer a solution that doesn't need this feedbackStateControl

[jomunker]

Okay. The other two options I can think of are:

1. Somehow infer from function if it is async or not
2. Add a second onClickAsync prop for the promise-based usage

[jomunker]

That's too late unfortunately. I could infer that a user wants to rely on outside state if loading and/or hasError is not undefined 🤷🏻‍♂️

[johnnyomair]

That's too late unfortunately. I could infer that a user wants to rely on outside state if loading and/or hasError is not undefined 🤷🏻‍♂️

I like this idea! Could you please try this? Also, I'd still recommend adding the promise detection in a separate PR: #2237 (review)

[jomunker]

Added a new PR for the changes to CrudContextMenu: #2423
Was easier to do it like this since the current PR targets next. I'll add another one for the addition of the promise feature.

[jomunker]

Here to following PR: #2425

[jomunker]

I will close this one

[johnnyomair]

Suggested change

	hasErrors?: undefined;
	loading?: undefined;
	hasErrors?: never;
	loading?: never;

[jomunker]

4f4b410

[johnnyomair]

This would need to be optional to not be a breaking change

[jomunker]

34316b6

[jomunker]

Closed because it was split in PRs #2423 and #2425