Closed jcheringer closed 1 month ago
@jcheringer Thank you for contributing to this open-source project! I'm excited to see a library that enables people worldwide to use Gravatar profiles. This is a fantastic library with great potential for expanding the use of Gravatar profiles. To ensure we offer a great DX, I'd like to start the review by focusing on the API design.
I was thinking we could offer an open method, e.g., quickEditor.open()
, instead of setting a single trigger element and handling the click event internally. This method would allow the user to open the quick editor at any place and time point they choose.
I noticed that we can update the user's avatar by parsing and modifying the img element's src attribute internally. However, there are additional edge cases we need to consider, such as validating the Gravatar URL and checking if we are overriding the existing URL parameter.
I suggest we provide relevant events, such as onProfileUpdateSuccess
and onProfileUpdateError
, to allow developers to perform their desired actions. This way, we can avoid directly modifying the img
elements.
local
optionWe could offer a local option to allow users to access the Quick Editor in their preferred language.
Based on the above points, I'd like to share my picture for the library. Please let me know your thoughts bro, thanks!
API:
const quickEditor = new GravatarQuickEditor( {
email: '...', // If it is available at the time of initialization
scope: [ 'avatars' ],
local: 'en', // "en" by default
onProfileUpdateSuccess: ( type ) => {
// Developers can handle the success event here. E.g:
// - Update avatars
// - Call the profile REST API to update the data
// - Close the popup window
// - And more...
},
onProfileUpdateError: ( type ) => {
// Developers can handle the error event here.
},
onOpened: () => {
// (Good to have) Developers can handle the event when the popup window is opened.
},
onClosed: () => {
// (Good to have) Developers can handle the event when the popup window is closed.
},
} );
// Open the popup window at anytime and anywhere
quickEditor.open( email /* If it's available at the time of opening */ );
// Close the popup window at anytime and anywhere
quickEditor.close();
Types:
export type Scope = ( 'avatars' | 'profiles' | '...' )[];
export type OnProfileUpdateSuccess = ( type: string ) => void;
export type OnProfileUpdateError = ( type: string ) => void;
export type OnOpened = () => void; // Good to have
export type OnClosed = () => void; // Good to have
export type Options = Partial< {
email: string;
scope: Scope;
local: string;
onProfileUpdateSuccess: OnProfileUpdateSuccess;
onProfileUpdateError: OnProfileUpdateError;
onOpened: OnOpened; // Good to have
onClosed: OnClosed; // Good to have
} >;
export type Open = ( email?: string ) => void;
export type Close = () => void;
Hey, @wellyshen! Thanks for the review and detailed explanation.
I've adjusted the overall structure to follow your suggestions. We now have two primary classes for the developers to use:
GravatarQuickEditorCore
has the core functionality, and it's close to what you suggested, allowing the developers more granular control over the functionalities.
GravatarQuickEditor
aims to simplify and streamline developers' implementations. This class will set up event listeners based on the selectors passed to the constructor and auto-refresh the avatars on the page if the appropriate selector is given.
Let me know what you think.
Thanks for the second round of review, @wellyshen! Answering to some of your questions:
Do you think it's worth providing a close method (and the related event) for developers? The use case I can think of is allowing developers to close the popup window when the user opens other modals / popups or leaves the page, etc.
I'm not sure we should allow a developer to close the popup. Imagine a user is using the Quick Editor, and out of nowhere, the popup closes because the developer called the close method. That would be a poor experience.
Currently, the way we use it is to initialize the class after developers have gotten the user's email. Do you think we could provide an init( email?: string ) (or run( email?: string )) method for developers to run the relevant code at the desired time?
I don't think that's the purpose of that class. If a developer wants more control, they could use the GravatarQuickEditorCore
.
Make the relevant development things work, e.g. package.json scripts, etc.
Maybe that's something we could address in a follow-up PR?
I'm not sure we should allow a developer to close the popup. Imagine a user is using the Quick Editor, and out of nowhere, the popup closes because the developer called the close method. That would be a poor experience.
We could offer this feature and let developers decide how to use it in their specific cases. One more possible use case is that developers could use it to automatically close the popup once the user's avatar has been updated, instead of requiring the user to close it manually. However, if you think it's not necessary at this stage, I'm okay with leaving it out. We can add it when needed.
I don't think that's the purpose of that class. If a developer wants more control, they could use the GravatarQuickEditorCore.
Let's validate it when we integrate the library with other products.
Maybe that's something we could address in a follow-up PR?
No problems!
Proposed Changes
This PR introduces a new package:
@gravatar/quick-editor
Below is a usage example:
The constructor expects the following parameters:
triggerSelector
: A selector to the element that triggers the popup to be openedemail
: The user's email associated with the identity to be editedoptions
: Optional parameter with the following options:scope
: allows defining the sections we want to show in the Quick Editor.avatarSelector
: A selector to match the avatar images on the pageTesting Instructions
npm run start
web/packages/quick-editor/playground/
with your Gravatar profile data