-
Notifications
You must be signed in to change notification settings - Fork 3
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
Get ontologies by project iri #204
Conversation
I will try to do the review this week. |
@kilchenmann I will review this PR this afternoon. |
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 find the details in my comment.
If I can help with anything, please let me know.
@@ -119,6 +130,30 @@ export class AppComponent implements OnInit { | |||
); | |||
} | |||
|
|||
getAnythingOntologies() { |
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 you make this one method taking the ontology Iri as a parameter?
You could even make the parameter optional so it would query all ontologies metadata if the param is not provided.
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 can write a helper method getProjectOntologies
. The thing is I have to handle the response twice. One response is for anything project ontologies, the other one for dokubib project ontology. I need two separate variables here.
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.
@@ -60,6 +64,13 @@ export class AppComponent implements OnInit { | |||
|
|||
resourceStatus = ''; | |||
|
|||
itemPluralMapping = { |
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 this used for?
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 it for use with i18nPlural
?
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.
Yes, it's an example how we can use plural and singular notation and I can test in e2e if it's 3 ontologies
or 1 ontology
. I know, the js-lib is not the best place to use it, but I thought I want to test it in the test-framework.
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.
Here's the example: https://angular.io/api/common/I18nPluralPipe
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, got it. Thanks for the explanation.
@kilchenmann Thanks for the changes. I will check this PR again. |
@kilchenmann Is it ok if I look at this PR on Monday? |
It's okay. But I have to wait with the next steps... |
I will try to do it today then! |
I didn't want to stress you 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.
Ok, I think this PR is almost done. Please find my final remarks in the comments.
@@ -60,6 +64,13 @@ export class AppComponent implements OnInit { | |||
|
|||
resourceStatus = ''; | |||
|
|||
itemPluralMapping = { |
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, got it. Thanks for the explanation.
@@ -119,6 +130,30 @@ export class AppComponent implements OnInit { | |||
); | |||
} | |||
|
|||
getAnythingOntologies() { |
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.
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.
Thanks for this PR.
I have just spotted a missing semicolon, please see my comment.
Part of DSP-71
Resolves DSP-425
Feature
New method to get ontologies by project IRI. It returns an array of
OntologyMetadata
.Chore
Ignore yalc.lock, as it often caused merge conflicts and does not need to be under version control.