-
Notifications
You must be signed in to change notification settings - Fork 4
[NAE-1901] Taskref rendering update #203
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
Conversation
- added component resolution to data-field-template.component component
- modified resolveComponentPortal
- added abstract-base-data-field.component.ts - refactored data component attributes - corrected tests
- updated abstract components - modified data-field-template.component.html
- updated filter-data-fields-module
- updated filter-data-fields-module
# Conflicts: # docs/compodoc/components/js/search/search_index.js # docs/typedoc/components-core/modules.html
- modified task content rendering algorithm to keep taskRef in datafields when it is needed - created default component for taskref and update taskref components according to component register - updated data-fields.module.ts -
- fixing tests.
- added default component
- corrected test
- corrected test
- removed unnecessary component resolution - marked unused code as deprecated - implemented resolution for updateOn strategy
- corrected tests
- modified abstract classes according to test
projects/netgrif-components-core/src/lib/data-fields/boolean-field/models/boolean-field.ts
Outdated
Show resolved
Hide resolved
projects/netgrif-components-core/src/lib/data-fields/models/abstract-data-field.spec.ts
Outdated
Show resolved
Hide resolved
...ts/netgrif-components-core/src/lib/data-fields/multichoice-field/models/multichoice-field.ts
Outdated
Show resolved
Hide resolved
projects/netgrif-components-core/src/lib/registry/component-registry.service.ts
Outdated
Show resolved
Hide resolved
...netgrif-components-core/src/lib/task-content/task-content/abstract-task-content.component.ts
Outdated
Show resolved
Hide resolved
projects/netgrif-components/src/lib/data-fields/text-field/public-api.ts
Outdated
Show resolved
Hide resolved
- modified according to PR
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.
could we arrange a meeting to go over the high-level ideas of the changes? Due to the size of the PR, I was unable to fully grasp what is being done and what the ultimate goal of the changes is
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.
are docs/
files supposed to be part of the repository? I thought they were auto-generated
this.dataField = dataFieldPortalData.dataField; | ||
this.formControlRef = dataFieldPortalData.formControlRef; | ||
this.showLargeLayout = dataFieldPortalData.showLargeLayout; | ||
if (!this.formControlRef) { |
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.
formControlRef
is guaranteed to always be false
, since @Input()
s are first set during the OnInit
lifecycle hook
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 think in this case it is not guaranteed. It is possible, that dataFieldPortalData contains defined formControlRef that is provided through AbstractDataFieldTemplateComponent
and AbstractFieldComponentResolver
via datafield object.
But you are right it would be better to check whether datafield has registered formcontrol, I will change it.
if (!this.formControlRef) { | ||
this.formControlRef = new FormControl('', {updateOn: this.dataField.getUpdateOnStrategy()}); | ||
} | ||
if (!this.dataField.initialized) { |
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.
if the datafield was already initialized and we have created a new FormControl
is it a problem?
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 mentioned above, a I changed the check.
undefined, | ||
undefined, | ||
[{validationRule: 'requiredTrue', validationMessage: 'this is custom message'}]), | ||
formControlRef: new FormControl(), |
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.
must a FormControl
be set to the field? Is it not more natura for there to be no control yet?
return this._translate.instant('dataField.values.boolean.' + this.dataField.value); | ||
} | ||
|
||
private resolveErrorMessage(dataField: BooleanField, search: string, generalMessage: string) { |
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.
this implementation seems quite generic. Why is it specific only for a boolean field?
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.
This is refactored too in other PR, there it is managed as generic function.
@@ -0,0 +1 @@ | |||
<ng-template [cdkPortalOutlet]="componentPortal"></ng-template> |
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.
what is the point of this component? Can't the host just display the portal?
[showLargeLayout]="showLargeLayout"></nc-simple-text-field> | ||
</div> | ||
</ng-template> | ||
<!--<ng-template #textFieldTemplate let-showLargeLayout="showLargeLayout">--> |
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.
why leave this in?
editable: true, | ||
hidden: true | ||
}, undefined, []), | ||
formControlRef: new FormControl(), |
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.
FC
hidden: true | ||
}, undefined, | ||
undefined), | ||
formControlRef: new FormControl(), |
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.
FC
@@ -1,54 +1,4 @@ | |||
<div fxLayout="row" fxLayoutAlign="start center" class="netgrif-input" [ngClass]="{'min-row-height': isField() && !isCustomHeight()}" [ngSwitch]="getElementType()"> |
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.
can't the ngSwitch
be completely replaced with portals?
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.
DataGroup does not have a field implementation and we may want to deprecate the use of them in future resleases. Builder does not supports them neither. A fully portal-based solution would be better, so I decided to replace ngSwitch, and add ngIf for remiaining fields, then if the datagroup solution will be removed, we can remove the div too.
- corrected according to PR
# Conflicts: # docs/compodoc/components/js/search/search_index.js # docs/typedoc/components-core/modules.html
# Conflicts: # docs/compodoc/components/js/search/search_index.js # docs/typedoc/components-core/modules.html
- revert ngSwitch
- corrected according to tests.
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
SonarCloud Quality Gate failed.
|
🎉 All dependencies have been resolved ! |
Description
It is now possible to render taskref as list of tasks (aka. taskView).
Implements NAE-1901
Dependencies
No new dependencies were introduced.
Third party dependencies
No new dependencies were introduced.
Blocking Pull requests
Depends on #198
How Has Been This Tested?
This was tested manually and with unit tests.
Test Configuration
Checklist: