-
Notifications
You must be signed in to change notification settings - Fork 19
chore(components): update post avatar to handle errors if no gravatar is found #5860
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: main
Are you sure you want to change the base?
chore(components): update post avatar to handle errors if no gravatar is found #5860
Conversation
🦋 Changeset detectedLatest commit: 03d002e The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Related Previews |
…-errors-if-no-gravatar-is-found
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.
…-errors-if-no-gravatar-is-found
…-errors-if-no-gravatar-is-found
…-errors-if-no-gravatar-is-found
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.
Since it's working now, I'm fine with it generally.
However, I have two concerns:
- It's not only fixing a bug, but adding features, therefore, please update the changeset file or split the pr into a bugfix and a feature (if possible).
- Let @alizedebray review it as well, because I expect, she could be not fully ok with the change, since it is not exactly what she demanded for.
const GRAVATAR_DEFAULT = '404'; | ||
const GRAVATAR_RATING = 'g'; | ||
const GRAVATAR_SIZE = 80; | ||
|
||
function getGravatarUrl(email: string): string { | ||
const hash = cryptify(email.trim().toLowerCase()); | ||
return `https://www.gravatar.com/avatar/${hash}?s=${GRAVATAR_SIZE}&d=${GRAVATAR_DEFAULT}&r=${GRAVATAR_RATING}`; | ||
} | ||
|
||
async function cryptify(key: string) { | ||
return await crypto.subtle.digest('SHA-256', new TextEncoder().encode(key)).then(buffer => { | ||
return Array.from(new Uint8Array(buffer)) | ||
.map(bytes => bytes.toString(16).padStart(2, '0')) | ||
.join(''); | ||
}); | ||
} |
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.
It would be nice, if we could share these constants and logic somehow between the component and the e2e test.
For example store and export them in a dedicated file aside the post-avatar.tsx, so you can use the same source on both sides.
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.
What about that topic? Is something not clear?
I can't see any change, nor any message, why we should not do this.
…-errors-if-no-gravatar-is-found
Functionality Changes
|
…-errors-if-no-gravatar-is-found
|
if (!imageResponse?.ok) { | ||
return false; | ||
} | ||
if (imageResponse.ok) { | ||
this.imageUrl = imageResponse.url; | ||
this.imageAlt = `${this.firstname} ${this.lastname} avatar`; | ||
this.avatarType = AvatarType.Image; | ||
return true; |
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.
This should be an if/else statement.
if imageResponse.ok use it, else return false;
Or keep the return imageResponse.ok;
as it was.
this.slottedImage = this.host.querySelector('img'); | ||
this.getAvatar(); | ||
connectedCallback() { | ||
this.getAvatarInitials(); |
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 add a comment in the code to explain, why this is necessary.
} | ||
|
||
componentDidLoad() { | ||
componentWillLoad() { |
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 think there was a reason, why we had to validate only on componentDidLoad
.
Have a look into the other components to find out, if this change should be reverted.
} | ||
}); | ||
this.getAvatarImage(); | ||
} |
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.
} | |
} | |
let imageResponse: Response; | ||
try { |
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.
let imageResponse: Response; | |
try { | |
let imageResponse: Response; | |
try { |
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 add some empty lines between var definition and if statements which do not belong together, in getAvatarImage
function, to improve readability.
@Watch('userid') | ||
validateUserid() { | ||
checkEmptyOrType(this, 'userid', 'string'); | ||
this.getAvatarImage(); | ||
} | ||
|
||
@Watch('email') | ||
validateEmail() { | ||
if (this.email) checkEmptyOrPattern(this, 'email', emailPattern); | ||
this.getAvatarImage(); | ||
} |
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.
Leaving this as is, and calling the validateUserId
and validateEmail
function on componentWillLoad
causes the getAvatarImage
function to be called three times on startup.
- In the
connectedCallback
(which is the only one that should be called initially) - In
validateUserId
triggered bycomponentWillLoad
- In
validateEmail
triggered bycomponentWillLoad
Please make it possible to call the validation functions without triggering the getAvatarImage
.
For example:
// like so, `updateUserId()` is only called when 'userid' gets an update
@Watch('userid')
updateUserid() {
this.validateUserid();
this.getAvatarImage();
}
// while the `validateUserid()` function can be called initially and when the `userid` chages
private validateUserid() {
checkEmtpyOrType(this, 'userid', 'string');
}
connectedCallback() {
// this should be the only inital call to `getAvatarImage()`
this.getAvatarImage();
}
componentDidLoad() {
// initially only call the validation
this.validateUserid();
}
cy.get('@avatar').find('img').should('not.exist'); | ||
cy.get('@avatar').find('.initials').should('exist'); | ||
}); | ||
|
||
it('should not show image bur fallback to initials, when slotted image is invalid', () => { |
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.
it('should not show image bur fallback to initials, when slotted image is invalid', () => { | |
it('should not show image but fallback to initials, when slotted image is invalid', () => { |
'append', | ||
'<img src="/assets/images/invalid-image.svg" alt="Invalid image" />', | ||
); | ||
cy.get('@avatar').find('.initials').should('exist'); |
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.
You should also check if the slotted image is not visible
const GRAVATAR_DEFAULT = '404'; | ||
const GRAVATAR_RATING = 'g'; | ||
const GRAVATAR_SIZE = 80; | ||
|
||
function getGravatarUrl(email: string): string { | ||
const hash = cryptify(email.trim().toLowerCase()); | ||
return `https://www.gravatar.com/avatar/${hash}?s=${GRAVATAR_SIZE}&d=${GRAVATAR_DEFAULT}&r=${GRAVATAR_RATING}`; | ||
} | ||
|
||
async function cryptify(key: string) { | ||
return await crypto.subtle.digest('SHA-256', new TextEncoder().encode(key)).then(buffer => { | ||
return Array.from(new Uint8Array(buffer)) | ||
.map(bytes => bytes.toString(16).padStart(2, '0')) | ||
.join(''); | ||
}); | ||
} |
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.
What about that topic? Is something not clear?
I can't see any change, nor any message, why we should not do this.
📄 Description
This PR updates the
<post-avatar>
component to better handle cases where no Gravatar image is found, with the following:getImageByProp()
function now first checks for a cached failed image url to avoid unnecessary fetch.userid
andemail
props to track changes and refresh the avatar accordingly.📝 Checklist