-
Notifications
You must be signed in to change notification settings - Fork 4
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
Refactor how to get properties by type #88
Conversation
As long as there isn't a new RC for the JS lib, it has to be built manually.
|
@flavens Hopefully Non Breaking News: @knora/api@1.0.0-rc.1 is out!!! |
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.
Please see my comments.
projects/dsp-ui/src/lib/viewer/views/resource-view/resource-view.component.ts
Show resolved
Hide resolved
projects/dsp-ui/src/lib/viewer/views/property-view/property-view.component.spec.ts
Show resolved
Hide resolved
@@ -1,8 +1,8 @@ | |||
<div class="resource-view" *ngIf="resource"> | |||
|
|||
<!-- dsp-property-view --> | |||
<div *ngIf="propArray.length !== 0; else noProperty"> | |||
<dsp-property-view [parentResource]="resource" [propArray]="propArray" [systemPropArray]="systemPropArray"> | |||
<div *ngIf="resPropInfoVals.length !== 0 || systemPropDefs.length !== 0; else noProperty"> |
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.
Is this check necessary? How would PropertyViewComponent
behave if both resPropInfoVals
and systemPropDefs
are empty arrays?
You don't have to worry about not initialised members because bot arrays are initialised with an empty array. I think that's a good practice.
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.
ok so I could remove this check but we should manage somehow the case where we get empty arrays, the user should know that there is no properties
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.
Ok, fine with me if you leave this check.
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 mean, the current way it is implemented is fine :-)
projects/dsp-ui/src/lib/viewer/views/property-view/property-view.component.spec.ts
Show resolved
Hide resolved
projects/dsp-ui/src/lib/viewer/views/resource-view/resource-view.component.ts
Show resolved
Hide resolved
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.
Ok, looks good!
For more info: DSP-142 and DSP-275