Hovercards: Handle default header image #221
Conversation
|
Size Change: +580 B (+0.89%) Total Size: 66.1 kB
|
wellyshen
left a comment
There was a problem hiding this comment.
Not related to this PR: I’m not sure the header image height matches the design. It appears different between the card and the profile.
| Profile | Card |
|---|---|
| transform: translateX(-50%); | ||
| overflow-y: hidden; | ||
|
|
||
| img { |
There was a problem hiding this comment.
In the lib, we use the class name instead of the element so that developers can override styles in a more DX-friendly way :)
There was a problem hiding this comment.
Is this really necessary?
A developer could easily override the CSS using .gravatar-hovercard__header-image img { }.
To add a CSS class to the element we would need to 'create' a class name that might not make much sense, like .gravatar-hovercard__header-image-img or .gravatar-hovercard__header-image-background.
Or we would need to change the current CSS structure to create a CSS hierarchy that makes sense, like:
<div class="gravatar-hovercard__header-image-wrapper">
<img class="gravatar-hovercard__header-image" />
</divAnd that would require changes in the code that would make it less readable and organized.
In the end, the benefit, if any, would be a developer writing 4 or 5 characters less.
.gravatar-hovercard__header-image img { }
.gravatar-hovercard__header-image-img { }
.gravatar-hovercard__header-image-background { }
.gravatar-hovercard__header-image { } // a few less characters but would require structural changesThere was a problem hiding this comment.
From a library perspective, classes are the best practice: predictable specificity and refactor-safe. That’s why we use class names instead of element selectors 😅
There was a problem hiding this comment.
I'm not entirely convinced this is the best solution for this particular situation, but I made the changes to avoid blocking the PR.
| jobTitle?: string; | ||
| company?: string; | ||
| headerImage?: string; | ||
| hideDefaultHeaderImage?: boolean; |
There was a problem hiding this comment.
In the library, ProfileData was designed to mirror the API payload. I’m fine not adding an additional opt-out option here. If the user doesn’t have a header image, we can show the blurred placeholder directly, which is more aligned with Gravatar profiles. WDYT?
There was a problem hiding this comment.
In the profile editor and profile page, we added an option for users to hide the default header image if they prefer.
We need to add the same option on the Hovercards to be consistent.
There was a problem hiding this comment.
What, I didn't know that, where is the option?
There was a problem hiding this comment.
Oh, is there. Now the prop makes sense!
The height difference is intentional. It was designed this way to maintain the image's proportion, so we don't have to crop it. |
There was a problem hiding this comment.
LGTM, feel free to move forward once the consistent CSS selector issue has been addressed.
There was a problem hiding this comment.
@jcheringer Oops! I forgot to mention. To allow the editor to show or hide the hovercard’s default header image. We also need to pass the new data (from the API payload) to the method, from here.
Proposed Changes
This PR updates the Hovercards to show a default header image when the user doesn't have an uploaded image.
It also introduces the field
hideDefaultHeaderImageto allow users to hide the default header image.The field is opt-out by default, which means it will show a default header image (when appropriate) unless the user chooses to opt out.
Testing Instructions
hideDefaultHeaderImageto true - the affected card should not show the default header image now