-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add option to choose which type of image to show in changelog #166
Add option to choose which type of image to show in changelog #166
Conversation
<!-- TODO gravatar? MS Office 365? --> | ||
<img src="/assets/images/default_avatar.png" class="img-circle"> | ||
<div class="imageContainer"> | ||
<img class="image1" src='{{getDefaultPicture(change.principal)}}'> |
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.
Should be an Observable, no direct method invocation (Angular will call that method a dozen times)
<img src="/assets/images/default_avatar.png" class="img-circle"> | ||
<div class="imageContainer"> | ||
<img class="image1" src='{{getDefaultPicture(change.principal)}}'> | ||
<img class="image2" src='{{getProfilePicture(change.principal)}}'> |
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.
See above
} | ||
|
||
getDefaultPicture(user: string) { | ||
switch (this.changelogDefaultPicture.toLowerCase()) { |
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.
Code could be combined with getProfilePicture (e.g. getPicture(user: string, pictureType: string)
)
|
||
|
||
|
||
|
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.
empty lines can be deleted
top: 0; | ||
left: 0; | ||
} | ||
.image1 { |
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.
"defaultImage" would be better in my opinion :)
height: 45px !important; | ||
border-radius: 50%; | ||
} | ||
.image2 { |
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.
see above => "profileImage"
getProfilePicture(user: string, pictureType: string) { | ||
if (pictureType === 'profile') { | ||
//TODO DELETE LOG : 40 times | ||
console.log('GET PROFILE PICTURE'); |
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.
:O
} | ||
} else if (pictureType === 'default') { | ||
//TODO DELETE LOG : 40 times | ||
console.log('GET DEFAULT PICTURE'); |
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 remove
<div class="imageContainer"> | ||
<img class="image1" src='{{getProfilePicture(change.principal, "default")}}'> | ||
<img class="image2" src='{{getProfilePicture(change.principal, "profile")}}'> | ||
</div> |
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.
why render two images here? I checked the look of this in the UI and i think this here is not how it should be
@@ -56,6 +60,10 @@ export class DashboardComponent implements OnInit { | |||
.pipe(flatMap(env => this.environments.getChangeLog(env.id))) | |||
.pipe(map(changes => this.formatChanges(changes, config.changelogEntries, config.changelogMinDays))) | |||
.pipe(shareReplay(1)))); | |||
firstValueFrom(this.serverInfoService.getUiConfig()).then(r => { |
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.
r => config better
@@ -1,5 +1,6 @@ | |||
package com.hermesworld.ais.galapagos.changes.config; | |||
|
|||
import com.hermesworld.ais.galapagos.uisupport.controller.ProfilePicture; |
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.
Dependency violation: A "config" package (or, the "changes" "module") should not be dependent on "uisupport" package. Please move ProfilePicture here, in this Config package.
const h = image.naturalHeight; | ||
const w = image.naturalWidth; | ||
if (h === 1 && w === 1 && image.src !== change.defaultPictureUrl) { | ||
image.src = change.defaultPictureUrl; |
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.
Instead of accessing img directly, you can set change.profilePictureUrl = change.defaultPictureUrl
here.
Need to be logged in to outlook in the same browser to user outlook images!