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

Stabilize BlockCanvas's contentRef property #67209

Closed
costasovo opened this issue Nov 21, 2024 · 9 comments
Closed

Stabilize BlockCanvas's contentRef property #67209

costasovo opened this issue Nov 21, 2024 · 9 comments
Assignees
Labels
[Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@costasovo
Copy link
Contributor

costasovo commented Nov 21, 2024

What problem does this address?

The contentRef property, which provides access to the editor canvas wrapping element, is currently available only in the ExperimentalBlockCanvas. This prevents third parties which use the BlockCanvas to access the canvas element via the ref.

The property is used in the VisualEditor component in wordpress/editor package to handle various visual effects, focusing blocks or handling clicks.

There is a similar use case in the Email Editor we are building for MailPoet. The editor is currently experimental, but we plan to release in the next couple of months so we want to eliminate usage of private APIs as much as possible.

Another example of usage is in IsolatedBlockEditor.

It seems that a couple of independent custom editors ended up using this property.

What is your proposed solution?

I propose exposing the contentRef property on the public BlockCanvas component PR:(#67210).

Stabilizing the contentRef prop was already proposed as part of #57672. I want to highlight the comment from @youknowriad.

BlockCanvas shouldIframe, iframeProps contentRef
I intentionally kept these props private. They were needed because of our weird implementations and backward compatibility needs in WordPress but from a third-party developer perspective, I would love for the iframe to be default (no need to provide a choice), and for the other props to be not needed. I didn't want to commit to them as API because they felt very unclear from an implementer's perspective.

It has been more than half a year since the proposal so I hope we could revisit this. Using ref/forwardRef is quite common so I wouldn't say it is unclear. My concern here that allowing access to a dom element as public API could be too powerful and hard to manage in terms of backward compatibility. What would be the alternative approach?

Pros

  1. ref forwarding is known React pattern
  2. it is easy to do

Cons

  1. exposing a dom node to third parties allows variety of modifications and we can't fully garant backward compatibility as any change in the dom can potentially break something. We could perhaps address this by limiting the ref via useImperativeHandle
@costasovo costasovo added the [Type] Enhancement A suggestion for improvement. label Nov 21, 2024
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Nov 21, 2024
@youknowriad
Copy link
Contributor

Cn you share what use-cases you're trying to achieve by exposing the content ref? Trying to understand the problem rather than the solution.

@costasovo
Copy link
Contributor Author

Cn you share what use-cases you're trying to achieve by exposing the content ref? Trying to understand the problem rather than the solution.

As for the email editor, the important use-case is handling clicking into the canvas. When the user clicks in the non-editable template area, we show a popup that allows switching to the template mode. We also use the click event for selecting a nearest editable block, and we also merge typewriterRef into the contentRef but these are rather nice to have which we could live without.

@youknowriad
Copy link
Contributor

I would love if we can solve these issues without necessarily making contentRef public as it's fairly low level.

When the user clicks in the non-editable template area, we show a popup that allows switching to the template mode.

We have something very similar in the editor package: We lock the template and handle double click on non-editable template areas to show a popup to switch to template mode. If I'm not wrong, you're using the editor package in the email editor. Is that behavior not something that worked for you?

@costasovo
Copy link
Contributor Author

We have something very similar in the editor package: We lock the template and handle double click on non-editable template areas to show a popup to switch to template mode.

The behavior in the email editor is the same. We basically created a simplified version of the VisualEditor component and the popup. We can't reuse the same component because we need to use different wording for the popup so that the message would fit the email editing context. Is it a good idea to add an API to replace the message?

If I'm not wrong, you're using the editor package in the email editor. Is that behavior not something that worked for you?

Yes, we are using the editor package, but we don't use the Editor component that wraps the full editor UI, but we use EditorProvider, and the store and we created custom visual editor, header, sidebars, etc.

I would love if we can solve these issues without necessarily making contentRef public as it's fairly low level.

I understand the concern here. We are thinking about migrating to the post editor, which would solve the issue. This will take some time. So, we need to figure out how to handle this private API before we are able to migrate. I guess we could access the element directly from the DOM, but I'm afraid that this is even less stable than staying with the private API.

@youknowriad
Copy link
Contributor

The behavior in the email editor is the same. We basically created a simplified version of the VisualEditor component and the popup. We can't reuse the same component because we need to use different wording for the popup so that the message would fit the email editing context. Is it a good idea to add an API to replace the message?

I think we have two options here:

  • We can make the message a more generic (maybe remove the mention of pages and posts)
  • We already have a "messages" API. In PHP custom post types can define a number of labels on the registration function. We can definitely add new messages there if needed.

@costasovo
Copy link
Contributor Author

I think we have two options here:

  • We can make the message a more generic (maybe remove the mention of pages and posts)
  • We already have a "messages" API. In PHP custom post types can define a number of labels on the registration function. We can definitely add new messages there if needed.

Thanks for listing the options. I was thinking about the messages API, but I realized that it is meant to define labels for a single CPT. But, the message in the popup is related to a template that can be used with multiple post types. I see that the block template is registered with the post_types property so perhaps we could use the list of post types from the property to generate the text in correct context.
There is basically the same issue in the EntitiesSavedStates component, which shows, This change will affect pages and posts that use this template.
So, I was thinking we could change the list of affected post types in the message dynamically based on these data. Would that sound like an acceptable solution? I tried to see if there was a GitHub issue related to this but couldn't find any. I'm happy to write one, just please let me know if it is a good idea.

I want to add that the solution discussed above would help us only after we switch to the native post editor or perhaps to a higher-level component, e.g., Editor from the editor package. As for now, we use BlockCanvas directly, so the current problem is basically a mechanism for triggering the modal. As we might switch to the native editor, I think that we could use the hacky workaround for now. I would close this issue and focus rather on the messages to make the native editor more suitable for our use case.

@youknowriad
Copy link
Contributor

So, I was thinking we could change the list of affected post types in the message dynamically based on these data. Would that sound like an acceptable solution?

Yes, I think it sounds like a good solution to me. (depending on the complexity of the implementation). I don't think there's an issue for it. Even a generic message without mentioning the post types could maybe achieve the job better than the current message like.

This change will affect other parts of your site that use this template.

@ellatrix
Copy link
Member

It sounds like this can be closed? I also think that we should revise for what the contentRef is needed and build it whatever uses it into the canvas maybe. I agree that it should not be exposed.

@youknowriad
Copy link
Contributor

Closing this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants