Skip to content
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

[DEV-13852] Refactoring - Allow custom rendering of placeholders #574

Merged

Conversation

kudlajz
Copy link
Contributor

@kudlajz kudlajz commented Nov 5, 2024

EDIT: I have reworked all of the placeholder components that were using SearchInputPlaceholderElement to support custom placeholder rendering.

The main idea behind this refactoring is to simplify the rendering of placeholders, allowing full control over the contents of the placeholders as soon as they're clicked and to also improve the developer experience when adding or modifying the existing UI. Since a lot of the placeholder code will now be part of the consumer codebase, we don't have to release new versions of the editor when doing copy changes for example.

I chose the GalleryBookmarkPlaceholderElement as a proof of concept since it also contains the site switcher. The plan is (if we agree to it) to rework the remaining "search input" placeholders and then eventually rework the regular "input" placeholders as well.

The interface has been simplified into sort of a "headless" approach, so now the consumer only needs to provide a renderPlaceholder function that will receive different props based on the required logic.

This allows full control over the UI and also allows usage of consumer code's UI components instead of having to rely on the UI components present (or not present) in the editor. This way, we can get rid of things like invalidateSuggestions etc. that were more of a band-aid to the current solution rather than a proper fix.

In this case, the gallery bookmark placeholder rendering function will receive props like onSelect (when an option is selected) and onRemove (when we want to remove the placeholder).

For example, the onSelect callback expects a promise that will resolve to an object containing the OEmbedInfo of a given gallery, so we no longer need to provide fetchOembed to the editor and we can do all of this outside in the app.

You can see how the new approach is implemented in https://github.com/prezly/prezly/pull/15756.

Please also read the comment below and provide your thoughts on this approach.

@kudlajz kudlajz self-assigned this Nov 5, 2024
Comment on lines 89 to 93
renderFrame={
isCustomRendered
? () => renderPlaceholder({ onRemove: handleRemove, onSelect: handleSelect })
: undefined
}
Copy link
Contributor Author

@kudlajz kudlajz Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still wondering if it's a good idea to have this half-approach where the "initial" placeholder UI is rendered by the editor, but then the subsequent UI is custom rendered.

I think ideally it would be nice if we could render the whole placeholder outside of the editor (i.e. rendered via renderPlaceholder function) but this would also mean that we would need to implement drag & drop functionality and other things (loading states, placeholder management etc.), so I wasn't sure if we should do it now or at all.

EDIT: We probably don't need to worry about placeholder management outside of the editor. It would still be internally managed by the editor as needed when using the respective callbacks.

The advantage of this "headless" approach is that we would have a complete control over the UI of the placeholders within the app and it will also allow other developers to bring their own UI (currently we force the placeholders to look the way they do, which I don't think is great).

Just wanted to hear your thoughts and opinions before I do the work, in case you don't think it's a good idea.

What do you think?

Copy link
Contributor

@e1himself e1himself Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally dislike how these extensions are structured with the code split between the app repo and the editor repo. Making the extension hard-to-use outside of the Prezly app. But hey! One step at a time! :)

With this refactoring I can definitely see that things become less complicated. And even though the code splitting issue is still there, it is a great improvement 👍

@kudlajz kudlajz changed the title [DEV-13852] Refactoring - Allow fully custom rendering of placeholders [DEV-13852] Refactoring - Allow custom rendering of placeholders Nov 5, 2024
Copy link
Contributor

@fgyimah fgyimah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏾

@kudlajz kudlajz merged commit ffea45c into main Nov 8, 2024
9 checks passed
@kudlajz kudlajz deleted the feature/dev-13852-refactor-gallerybookmarkplaceholderelement branch November 8, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants