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

[Dialog] Add event before closing panel to allow validation of the data provided by the user #2614

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

diegomodolo
Copy link

…ta. (#1508)

Pull Request

📖 Description

I've added functionality to prevent the panel from closing if rules defined by the user don't pass.
I've added a func that will be called when the user clicks the button to commit the changes (won't be called on cancel or dismiss), and that func will return true or false, accordingly to the user validation rules.
image

🎫 Issues

#1508

👩‍💻 Reviewer Notes

📑 Test Plan

✅ Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new component
  • I have modified an existing component
  • I have validated the Unit Tests for an existing component

⏭ Next Steps

@diegomodolo
Copy link
Author

diegomodolo commented Sep 3, 2024 via email

@diegomodolo
Copy link
Author

diegomodolo commented Sep 3, 2024 via email

@vnbaaij
Copy link
Collaborator

vnbaaij commented Sep 3, 2024

Nice work! We need to take a closer look of course, but on first quick glance this look good and simple.

We are on the brink of releasing a new version. If we approve and merge, this will go into the next one then.

@dvoituron
Copy link
Collaborator

Interesting.

Why not use OnClosing? Possibly by adding a Cancelled parameter.

I also find the On suffix indicates that this is an EventCallback, which is not the case.

@diegomodolo
Copy link
Author

Interesting.

Why not use OnClosing? Possibly by adding a Cancelled parameter.

I also find the On suffix indicates that this is an EventCallback, which is not the case.

I thought about the parameter, but I would have to add it to DialogInstance, and I thought it would be more confusing since the property would not relate to the context of the class.

About the On prefix, I can change it, no problem.

@dvoituron
Copy link
Collaborator

You could add this parameter to a inherited class of DialogInstance (like DialogInstanceClosing).

My question is more: why to have similar "events". A
Func<bool> like you did can solve this problem, but using a Closing event can be more intuitive, no? 🙂

@diegomodolo
Copy link
Author

You could add this parameter to a inherited class of DialogInstance (like DialogInstanceClosing).

My question is more: why to have similar "events". A Func<bool> like you did can solve this problem, but using a Closing event can be more intuitive, no? 🙂

I totally agree with you, it would make much more sense.
I will abandon this PR, make the changes and create a new one.

@dvoituron
Copy link
Collaborator

My reflection is more like used by the old (but excellent) WinForms 😁

https://learn.microsoft.com/en-us/dotnet/api/system.windows.forms.form.formclosing

@dvoituron
Copy link
Collaborator

I will abandon this PR, make the changes and create a new one.

No keep opened this PR until you find another solution. May be my idea is false and your PR will be better.

@vnbaaij vnbaaij added this to the v4.10.1 milestone Sep 6, 2024
@lsurma
Copy link

lsurma commented Sep 12, 2024

@diegomodolo
Wouldn't it make more sense to have an asynchronous version of this 'OnDialogValidation'?
Func<Task<bool>> OnDialogValidationAsync
Most of the time, some validation logic will require async calls to DB, API etc, so having sync callbacks only, seems not to be enough (at least for me).
Or am I just missing something and it's possible to call some async method right now?

My question is more: why to have similar "events". A Func<bool> like you did can solve this problem, but using a Closing event can be more intuitive, no? 🙂

@dvoituron And I feel the same way. The OnDialogValidation event/callback seems to be a really specific use case (but I agree that it could be used a lot), and the components in the core library should stay in a more generic way, and then maybe some more specific use cases should be implemented using the composition approach.
Because now, if I'm right, the validation behaviour could be achieved using the OnDialogClosing event callback, and if the closing should be stopped then inside that callback we should throw an exception (which is far from perfect, I guess, but it works).

but it works

After testing, I see that it doesn't work

@vnbaaij vnbaaij modified the milestones: v4.10.1, V4.10.2 Sep 26, 2024
@vnbaaij vnbaaij modified the milestones: V4.10.2, v4.10.3 Oct 10, 2024
@vnbaaij
Copy link
Collaborator

vnbaaij commented Oct 10, 2024

What is the status of this PR? Is it good to go as-is or do we want/still need to change anything?

@diegomodolo
Copy link
Author

Hi @vnbaaij , I would like to apologize for not being active at this thread.
It is my first contribution at a plublic repository like this, so I did not know the right steps.
I agree with the modification proposed by @lsurma to make the event async and I will make the changes.
Regarding @dvoituron proposal, I agree that having everything on the same event would make more sense, I did not develop it that way because I did not want to "mess" with the original code.

Anyway, I would like to know the correct steps to amend my PR, if anyone could help me.

@vnbaaij
Copy link
Collaborator

vnbaaij commented Oct 11, 2024

No need to apologize. Your contributions are always appreciated!

Just do the changes you want to make in your branch, push them to this PR and let us know know if and when you want us to review stuff.

@AndreasTC
Copy link

Hi @vnbaaij, @dvoituron & @diegomodolo,
I was wondering it there already is an update of when these changes will be released? 😊

@vnbaaij
Copy link
Collaborator

vnbaaij commented Oct 17, 2024

No news from our side. Thin @diegomodolo stille wants to make some changes to this PR. So it is not ready yet.

We never give dates or put dates on releases. We release when we think a release is ready.

@vnbaaij vnbaaij modified the milestones: v4.10.3, v4.11 Oct 18, 2024
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.

5 participants