-
Notifications
You must be signed in to change notification settings - Fork 32
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
Fix gravatar service calls #712
Conversation
remove the subscription in the constructor which is error prone, improve the service so it returns observables
Affected libs:
|
detected via e2e tests
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 the fix and explanation @fgravin ! I've adapted the unit tests and added a small fix detected by the e2e tests. Works fine in webcomponents, datahub and editor on my side.
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.
Great @tkohr thank you for taking over it.
I made a small comment, you can merge though.
@@ -12,7 +12,7 @@ export class GravatarService implements AvatarServiceInterface { | |||
private GRAVATAR_IDENTICON = 'mp' | |||
|
|||
private readonly identicon$ = this.gn4SettingsService.identicon$.pipe( | |||
map((identicon) => identicon.replace('gravatar:', '')) | |||
map((identicon) => identicon?.replace('gravatar:', '')) |
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 then is the undefined
value is handled in the subscription of this observable ?
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, I think l.25 below should handle it
map((identicon) => identicon || this.GRAVATAR_IDENTICON), |
The gravatar service fetches the GN4 settings in the constructor, and then provides sync properties (
profileIcon
,placeholder
), whichThe PR refactors the service to returns Observables instead of values, which are triggered only if a subscription is done.
This means that the fetch for the avatar setting does not occur in web components.