Skip to content

Conversation

@campersau
Copy link
Contributor

Add support for onclose event

Similar to #24036

Description

Reference: https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/close_event

There is also a cancel event for <dialog>, should it be added as well?

Fixes #50513

@campersau campersau requested a review from a team as a code owner September 15, 2023 00:32
@ghost ghost added area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member labels Sep 15, 2023
@ghost
Copy link

ghost commented Sep 15, 2023

Thanks for your PR, @campersau. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

void OnClose(EventArgs e)
{
message += "onclose,";
StateHasChanged();
Copy link
Member

Choose a reason for hiding this comment

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

This call shouldn't be needed, since the event dispatcher causes the equivalent to StateHasChanged implicitly anyway. Would be good to update this file so that the tests validate it works without explicitly calling this.

Copy link
Member

Choose a reason for hiding this comment

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

I'm just going to remove it now, because if it works and the tests pass, we can still merge this in time for .NET 8. If it misses the end-of-day deadline today, it will slip into .NET 9!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied it from the ToggleEventComponent maybe there are more unnecessary StateHasChanged in other test components as well.

@SteveSandersonMS SteveSandersonMS changed the base branch from main to release/8.0 September 15, 2023 08:44
@SteveSandersonMS SteveSandersonMS changed the base branch from release/8.0 to main September 15, 2023 08:44
@SteveSandersonMS
Copy link
Member

There is also a cancel event for , should it be added as well?

Ideally yes

@SteveSandersonMS
Copy link
Member

I'll get this finalized in time for .NET 8, so need to close this PR in favor of #50727. Your commit will be preserved, though.

@campersau campersau deleted the blazoroncloseevent branch September 15, 2023 08:55
@campersau
Copy link
Contributor Author

There is also a cancel event for , should it be added as well?

Ideally yes

Should I add it as well or are you going to add it in the other PR?

When you add both oncancel and onclose eventhandlers and hit the Escape key then both events will be called in this order oncancel, onclose.

@ghost
Copy link

ghost commented Sep 15, 2023

Hi @campersau. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@SteveSandersonMS
Copy link
Member

Should I add it as well or are you going to add it in the other PR?

I've added it in #50727

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Blazor] Add support for onclose event for dialog

2 participants