-
Notifications
You must be signed in to change notification settings - Fork 354
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
feat: [M3-8158] - Begin to sunset Gravatar #10859
feat: [M3-8158] - Begin to sunset Gravatar #10859
Conversation
2a5aa36
to
ae25868
Compare
93c8efe
to
ccea1bd
Compare
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 is looking great! some improvement can still be made but I really like the placeholder improvement 💅 and code cleanup 🧹
packages/manager/src/features/NotificationCenter/NotificationCenter.styles.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Profile/DisplaySettings/AvatarColorPickerDialog.tsx
Show resolved
Hide resolved
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 am confused. This change did fix the CI failure on the test-manager
job, which is now passing here, but it was a failure on a component not touched in this PR. There still seems to be something more going on here because Code Coverage / current branch is still failing. Local runs of unit tests don't reproduce errors.
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.
Seeing the same thing here. Test suite passes locally by CI is unhappy
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.
Reverted this change so it can be addressed in its own PR (M3-8559).
…CI?" This reverts commit c373a53.
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 work! Confirmed Avatar/Gravatar behaviors are working as expected and async rendering is handled as best as possible.
Thanks for working with me on improving the flickering issues even if this feels like micro-improvements and who knows, the 2 weeks period may be expended and we have a good UI to support it!
Can't wait to remove a lot of logic related to having to fetch the gravatar and kiss goodbye to this unnecessary burden 👋
Note: fine skipping the the failing test which has become a flake ❄️
export const GravatarByUsername = (props: GravatarByUsernameProps) => { | ||
const { className, username } = props; | ||
const { data: user } = useAccountUser(username ?? ''); | ||
const { data: user, isLoading } = useAccountUser(username ?? ''); | ||
const url = user?.email ? getGravatarUrl(user.email) : undefined; | ||
|
||
// Render placeholder instead of flashing default user icon briefly | ||
if (isLoading) { | ||
return <Box height={DEFAULT_AVATAR_SIZE} width={DEFAULT_AVATAR_SIZE} />; | ||
} | ||
|
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.
Latest change:
Adds isLoading
to render a placeholder for GravatarByUsername while we're waiting for the user
. This impacts the events landing and notification center gravatars. Without a placeholder here, while the query loaded, we were setting url
to undefined
, which rendered the default icon briefly and previously had me adding an extra long fade in (>1s) to avoid. With this actual fix, we still slightly (.2s) fade in the gravatar so the transition from loading to the icon is less abrupt.
Between this change and the cache in useGravatar
, we shouldn't be seeing anymore flickering, even at slow speeds.
Going to merge this now. The two failed tests in CI were unrelated and flaky - I've seen |
* Swap out gravatar for colored avatar on Profile > Display * Add utils to determine letter color * Bring back checkForGravatar event and fix hasGravatar boolean * Hide tooltip if user unless user hasGravatar * Add GravatarSunsetBanner.tsx * Use util for banner and profile page * Add Akamai wave icon for Akamai-generated user events * Clean up of theme colors and conditional rendering * Style ColorPicker * Try to handle constrast ratio * Address UX feedback: show avatar preview in dialog * Add new avatar to EventRow on Event Landing page * Conditionally render Gravatar in EventRow until sunset * Replace gravatar conditionally in UserRow of Users Landing * Conditionally render styled Avatar in UserSSHKeyPanel * Conditionally render Avatar in TopMenu; rename GravatarForProxy * Conditionally render styled Avatar in NotificationCenterEvent * Fix sunset date * Use MUI theme function to get contrasting text color * Clean up; change color default to darker color * Clean up Avatar, ColorPicker; add stories * Clean up and add unit tests * Clean up; fix test * Forgot to push last changes for Support; default color fix * Add changesets * Fix an accidentally skipped test * Address UX feedback: use 'Avatar' over 'Profile photo' * Address feedback: avoid regex * Fix bug: NotificationCenterEvent missing Linode system avatar * Use hook throughout gravatar replacement * improve useGravatar hook * Fix: showing new system/support avatars when user has Gravatar enabled * Experiment with component for loading/gravatar/avatar * Handle loading state and fade per-component to fix flickering * Fix username for support tickets; clean up * Fix avatar color for additional account users * Switch over to single GravatarOrAvatar component for rendering * Clean up commented code * Use const for default avatar size * Address feedback: use Map * Does not using the deprecated function fix the unit tests in CI? * Revert "Does not using the deprecated function fix the unit tests in CI?" This reverts commit c373a53. * Skip failing test with JSDom issues for now * Improve fading behavior * Revert "Improve fading behavior" because it causes other issues This reverts commit 04c0b6b. * Add GravatarByUsername loading placeholder; adjust other fades --------- Co-authored-by: Alban Bailly <abailly@akamai.com>
Description 📝
This is the first step in the two step process of sunsetting Gravatar. It adds new components and replaces the default icons for users currently without Gravatar with new avatars. For users with Gravatars, nothing changes yet except for an info dismissible banner informing Gravatar support will be sunset two weeks from the time this is released.
Changes 🔄
ColorPicker
component based on the native HTML ColorPickerAvatar
component to render a custom background color (stored in local storage) and first initial of usernameAvatarColorPickerDialog
to change the background color from its defaultAvatarColorPickerDialog
Avatar
component for users without Gravatars in the following places:Avatar
andAvatarColorPickerDialog
components; updatesEventRow
unit test for changesAvatar
andColorPicker
componentsTarget release date 🗓️
09/16/24
Preview 📷
More Photos 📸
How to test 🧪
Prerequisites
(How to setup test environment)
Verification steps
(How to verify changes)
Change Avatar Color
on http://localhost:3000/profile/display and the changes are reflected everywhere the Avatar is displayedyarn storybook
and check the Avatar and ColorPicker stories in StorybookAs an Author I have considered 🤔
Check all that apply