-
-
Notifications
You must be signed in to change notification settings - Fork 80
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
#578consistent header and footer #601
Conversation
@erinz2020 can you add "fixes #578 and fixes #579" in your PR description so this links to those issues? |
notifications = Collaboration.getNotificationsWidgetHtml(request, myShepherd); | ||
if (request.getUserPrincipal() != null && !loggingOut) { | ||
user = myShepherd.getUser(request); | ||
username = (user != null) ? user.getUsername() : null; |
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.
might be best to wrap this whole section in if (user != null) { ....
rather than test for username
separately, since we also should have one for the line below: if (user.getUserImage()...)
is not checking for null user
.
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. definitely a lot going on!
i put in one note about a missed null value check that should probably be fixed, but i think that is it.
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
ignoring this since we are moving beyond this. |
#578consistent header and footer
Fixes #578 and #579