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

Move CreateModal.Hide() to created/updated methods #7304

Merged
merged 4 commits into from Jan 20, 2021

Conversation

stsrki
Copy link
Contributor

@stsrki stsrki commented Jan 18, 2021

resolves #7291

@stsrki stsrki added this to the 4.3-preview milestone Jan 18, 2021
@stsrki stsrki requested a review from hikalkan January 18, 2021 09:45
@hikalkan
Copy link
Member

hikalkan commented Jan 19, 2021

We currently do 2 things after creating an entity:

  1. GetEntitiesAsync()
  2. CreateModal.Hide()

Now, we are moving the second one into OnCreatedEntityAsync(). I see a few problems with this approach:

  • Why not moving GetEntitiesAsync() also into the OnCreatedEntityAsync() (to be consistent)
  • OnCreatedEntityAsync() was designed as a hook point for end-developers. But currently, it performs some essential work. So, end-developer now much call base.OnCreatedEntityAsync() to preserve the existing behaviour while previously it was not necessary. This is a breaking change.

What do you think?

Edit: Alternatively, end-developer can completely override CreateEntityAsync() to customize it. It is not a long method.

@stsrki
Copy link
Contributor Author

stsrki commented Jan 19, 2021

@hikalkan I agree. Moving GetEntitiesAsync() into OnCreatedEntityAsync makes sense. The problem could potentially be in how users are using the OnCreatedEntityAsync. For example when whey override they have two options

protected override async Task OnCreatedEntityAsync()
{
   await base.OnCreatedEntityAsync();

  // do something
}
protected override async Task OnCreatedEntityAsync()
{
  // do something

   await base.OnCreatedEntityAsync();
}

If they expect GetEntitiesAsync() to already be called it could be a breaking change. So no.2 would not work in this case.

I can easily make changes. It's your call :)

@stsrki
Copy link
Contributor Author

stsrki commented Jan 20, 2021

@hikalkan Can you check PR now? Thanks

Copy link
Member

@hikalkan hikalkan left a comment

Choose a reason for hiding this comment

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

That's good now. Thanks!

@hikalkan hikalkan merged commit 292c0e2 into dev Jan 20, 2021
@hikalkan hikalkan deleted the stsrki/dev-crud-modal-hide branch January 20, 2021 09:20
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.

AbpCrudPageBase.cs
2 participants