-
Notifications
You must be signed in to change notification settings - Fork 3
story/VSPC-150 #370
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: develop
Are you sure you want to change the base?
story/VSPC-150 #370
Conversation
… page - Work in progress
… on slide page + Code cleanup
…aces-2.0 into story/VSPC-147
… them, fixed code indentations and div alignments
… shown on public page
…ography not detected by zotero extension
…detection by zotero - work in progress
…aphy, handling null cases of path variables, code cleanup
…rategy Design pattern
…ged Biblio controller and forms - Saving is working - Work in Progress
jdamerow
left a comment
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.
Also, there are codefactor issues and what happened to all the classes in the "references" package (e.g. https://github.com/diging/virtual-spaces-2.0/pull/299/files#diff-8e2f9f515ef07dfc360ae67df0401001f8cd6b89b960b99068f972b3fd962eec)
| reference.setEditors(editor); | ||
| reference.setType(type); | ||
| reference.setNote(note); | ||
| if(visibility == "Private") { |
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.
strings should not be hard-coded; at a minimum this needs to be a constant somewhere
|
|
||
| private String note; | ||
|
|
||
| private boolean visibility; |
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.
boolean variables should be adjectives so that the methods make sense when you call them is... (i.e. isVisibility does not make much sense...)
|
Make it so, Jenkins. |
| @RequestMapping(value = "/staff/module/{id}/slide/{slideId}/bibliography/{biblioId}/reference/add", | ||
| method = RequestMethod.POST) | ||
| public ResponseEntity<IReference> addReference(@PathVariable("id") String moduleId, | ||
| public ResponseEntity<Reference> addReference(@PathVariable("id") String moduleId, |
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 needed? why the change?
|
Guidelines for Pull Requests
If you haven't yet read our code review guidelines, please do so, You can find them here.
Please confirm the following by adding an x for each item (turn
[ ]into[x]).Please provide a brief description of your ticket
(you can copy the ticket if it hasn't changed)
Add Bibliography section to staff page
Like users or images, it should show all references sortable by author, year, title, or created date.
VSPC-150
Anything else the reviewer needs to know?
The previous PR for this branch 299 has a force push and hence cannot be re opened.