-
Notifications
You must be signed in to change notification settings - Fork 24
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
Enable publications to link to annotations #6315
Conversation
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.
Looks good already :) I added two small comments. Otherwise, Let’s wait for the frontend
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.
2 things I noticed in the GET /api/publications
API:
- A dataset contains the information about a publication, which is redundant.
- An annotation has the id in the form of
annotations: [{ "id": { "id": "...." }, ... }, ...]
, which seems to be unnecessarily nested.
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 need the name
of the annotations for the UI.
I would argue, we can remove the |
That’s good news, we weren’t sure if the front-end needs this in some other place (i.e. to link back to the publication from the dataset page or something). But if that is not needed, that will simplify the json and the backend code :)
That’s probably just the objectid wrapper, @leowe you can un-nest it by passing annotation._id.id to the json |
I fixed the annotation So, what is left is removing the |
@philippotto please review the frontend part :-) |
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.
the front-end looks great 👍 nice clean up! didn't test, though, since I don't know how to do that. maybe you could prepare a dev instance where some publications are available?
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.
Backend LGTM
Here you go: https://publicationsannotations.webknossos.xyz/ or https://publicationsannotations.webknossos.xyz/publications/5c766bec6c01006c018c7459 |
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.
awesome 🕺
URL of deployed dev instance (used for testing):
Steps to test:
http://webknossos/api/publications
Issues:
(Please delete unneeded items, merge only when none are left open)
Still todo