-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Moveout UI layer methods to new interface. #18356
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
base: main
Are you sure you want to change the base?
Moveout UI layer methods to new interface. #18356
Conversation
|
Thank you for submitting your first pull request, awesome! 🚀 If you haven't already, please take a moment to review our contribution guide. This guide provides helpful information to ensure your contribution aligns with our standards. A core team member will review your pull request. If you like Orchard Core, please star our repo and join our community channels. |
|
Accept the CLA |
|
@dotnet-policy-service agree |
@dotnet-policy-service agree |
Done |
|
While I also prefer not to use view models in service interfaces, I don't see much of a practical benefit to split the content definition operations like this (and thus have e.g. So, I don't think such a change is beneficial. But if you have really good arguments in favor of it, then please bring it up at one of the meetings and we can discuss it. BTW the file name and some fields storing instances of |
No No, you are right, IContentDefinitionViewModelService is not for service layer, is actually for UI layer. UI layer only usage view models thats why kept here. Actually IContentDefinitionService need to be move to the service/domain layer. Its contract should only expose domain-level operations (add/remove/update definitions), returning domain entities or DTOs, not ViewModels. And IContentDefinitionViewModelService in the UI layer should depend on IContentDefinitionService, and be responsible for mapping between domain entities ↔ ViewModels. AddPartAsync & RemovePartAsync are design flaw. AddPartAsync returning view model but RemovePartAsync working on to remove typedefination. We need to refactor on this. AddPartAsync must do following thing:-
So i refactor and push the changes:-
So its now a clear SoP. Please go through updated PR. Let me know still any confusion! Regards. |
I agree with @Piedone on this |
I also, thats why i break out ViewModels logics to different class. |
Just to let you know, all of us are busy with other work, but we are struggling to engage with the community. Second thing, you still didn't react to the last comment from @Piedone, do it or let us know your concern, and then you could request a review rather than mention the names |
Yeah thats fine. But Label attached seems very negative. Cheers. :) |
|
We usually use the label "don't merge" to avoid anyone from the team from merging this accidentally, unless the one who created it is convinced somehow |
As i seen from comment and code , that IContentDefinitionService contains methods related to UI + Non UI functionality. So i created new abstraction IContentDefinitionViewModelService which will handle only UI layer view models.
May be new interface not placed at right location, please feel free to change. As still i am going through a code.
So,
IContentDefinitionManager => handles caching, loading, storing, and building definitions.
IContentDefinitionViewModelService => handles UI layer view model
IContentDefinitionService => handles content management.
Please go through changes!
Regards.
Fixes #18235