-
Notifications
You must be signed in to change notification settings - Fork 22
Refactor(react-component): Architecture work #5247
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: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5247 +/- ##
==========================================
- Coverage 63.86% 63.84% -0.02%
==========================================
Files 1171 1171
Lines 82503 82517 +14
Branches 7407 7409 +2
==========================================
- Hits 52693 52686 -7
- Misses 29669 29690 +21
Partials 141 141
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general. But some questions if you can reply before approving it :)
private addEffect(effectFunction: () => void): void { | ||
this._disposables.push( | ||
this.addDisposable( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the same method addEffect
for the other classes are protected, can we change this one also to be protected (to at least be accessible in subclasses) or this one specifically needs to be private and why?
}) | ||
); | ||
this.addEffect(() => { | ||
this.renderTarget.revealSettingsController.renderQuality(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that it is not part of this refactoring scope but can we think somehow refactor this in order to follow the Demeter's Law?
}) | ||
); | ||
this.addEffect(() => { | ||
this.renderTarget.revealSettingsController.cameraControlsType(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that it is not part of this refactoring scope but can we think somehow refactor this in order to follow the Demeter's Law?
}) | ||
); | ||
this.addEffect(() => { | ||
this.renderTarget.revealSettingsController.cameraKeyBoardSpeed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that it is not part of this refactoring scope but can we think somehow refactor this in order to follow the Demeter's Law?
Type of change
Description 📝
Renames:
Rename
DomainObject.removeCore
toDomainObject.dispose
Rename
Views.clear
toViews.dispose
These names was misleading.
Synchronize:
Synchronize the code so all
DomainObject
,BaseComand
andRevealSettingsController
hasaddDisposable
andaddEffect
function. This saves lines and complexity. Used them where it was possible.How has this been tested? 🔍
Checklist ☑️