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

DataGrid : Makes sure to close the modal with explicit CloseReason.UserClosing, if Save ran to success #3185

Merged
merged 5 commits into from
Dec 5, 2021

Conversation

David-Moreira
Copy link
Contributor

Closes #3149

@David-Moreira David-Moreira requested a review from stsrki November 30, 2021 00:32
@stsrki
Copy link
Collaborator

stsrki commented Nov 30, 2021

Still not good. It now calls the event two times with the following result

image

Code:

PopupClosing="@((e)=>{Console.WriteLine(e.CloseReason); return Task.CompletedTask; } )"

@David-Moreira
Copy link
Contributor Author

Nice catch. I'll take a look.

@David-Moreira David-Moreira self-assigned this Dec 2, 2021
@David-Moreira
Copy link
Contributor Author

Apparently same happens with Cancel. Thing is we have two concurrent ways of Closing the modal, by API or by mutating the Visible Parameter workign at the same time.
Theorically it still should work, but it goes like this:

  • Hide call --> Reason: UserClosing
  • Concurrent SetParametersAsync still thinks visible is true, and runs visible set to true pipeline...
  • In the meantime Datagrid tells the correct visible state, false, and now runs the false pipeline... => Reason: None

Most likely the fix we're gonna work on, is to only use one of these ways in the DataGrid. Might be confusing to try handling the "concurrency" instead.

@stsrki
Copy link
Collaborator

stsrki commented Dec 4, 2021

I believe that's why I used the parameter Visible to control the Modal. If I remember correctly. If you want to control it properly, you will need to choose one way. Possibly with Show Hide would be the best.

@stsrki
Copy link
Collaborator

stsrki commented Dec 4, 2021

It is behaving well now. The only problem was that OpenModal and validations.ClearAll were called from sync code so I fixed it. You can check one more time to confirm and then I will merge it.

@David-Moreira
Copy link
Contributor Author

OpenModal should have still be async? It was on the AfterRender Task queue remember? :D
Validations.ClearAll was already wrong, ddin't notice, assumed it was a sync call.

@stsrki
Copy link
Collaborator

stsrki commented Dec 5, 2021

OpenModal should have still be async? It was on the AfterRender Task queue remember?

modalRef.Open was on AfterRender Task but validations.ClearAll was not. This is just a simple optimization to prevent any problem in the future.

@stsrki stsrki merged commit a8f7c54 into rel-0.9.5 Dec 5, 2021
@stsrki stsrki deleted the rel-0.9.5-DatagridPopupClosingSaveReason branch January 6, 2022 13:12
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