-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Block Bindings: Improve how bindings handle placeholder/fallback values #63442
Comments
How about adding a fifth option to prevent the block from rendering when its bound value is undefined? This could be implemented by adding a "whenUndefined" property to the bindings object. This property would specify how the block should behave when it encounters an undefined value. The structure could be as follows: "metadata": {
"bindings": {
"content": {
"source": "core/post-meta",
"args": { "key": "my_custom_field" },
"whenUndefined": "omitBlock" | "useOriginalContent"
}
}
} |
I think there are various possibilities to consider here. For example:
We should likely outline all of these possibilities to get a better handle on what the user experience should be like in those cases — like how it might look for the Post Meta, and how that might compare and contrast with the approach for custom sources. One idea: If a value is bound to the Post Meta source and editable, maybe the placeholder should be a message like, “Customize {attributeName}“. I’m thinking about the use case Carlos mentioned in an unrelated issue about having Block Bindings serve as a means of creating a template full of editable fields for users to override. I'll aim to to think through those possibilities and follow up this week if someone doesn't get to it before me. I added the "Needs Design Feedback" label as well since design may want to share feedback on this part of a potentially key user experience flow, though feel free to remove it if we feel that's not needed. |
I think this is an interesting and totally valid use case, although I wouldn't consider it exactly as another alternative but as another functionality to keep in mind. While I think it is something that should be supported at some point, it seems a bit more complex than it sounds to me:
I agree that we have to keep all the possibilities in mind, and I briefly touch on that in the options suggested. Basically, the main question is how sources define the placeholder when
I would say this is a different discussion. If a value is bound to Post Meta and editable, it means that it was able to "connect" to the source and there is no "undefined" value, so it would show the value of the custom field. Something like that could make sense when the value is empty (not undefined), but I see it as a different use case. |
@SantosGuillamot Ah ok! I misunderstood this a bit then, thanks. In that case, how about we keep the In our case for Post Meta, maybe we could do something like, "Binding unsuccessful. Please make sure {key} has been registered and is available to the REST API." That may be too verbose, but I think you get the idea 😄 What do you think? |
Do you mean adding a default for all sources or let each source define the default in the server? If it's the latter, I'm not convinced about having two different public APIs for handling the placeholder, to be honest. Lately, I'm slightly inclined to option 3, having a But I'm not 100% convinced either. |
Putting aside all the technical details, what is the user experience we are trying to build here? I see the following scenarios for the attribute bound to the source:
Pattern Override is an edge case because we always present the value provided in the designed pattern or the one that the user provided locally, so unless there would be some permission model introduced, we can keep it outside of the exercise. It's either the default value, or the edited value in the UI. Now the question to confirm is what happens if the value resolved on the server is an empty string. Does it stil replace the fallback value that might be set for the attribute in the saved HTML? I assume that it still does. Now, for any core block if the value for the attribute presented in the block canvas is empty, we provide some sort of placeholder, like for an Image block it's a media placeholder, for Paragraph or Heading block, the RichText provides the placeholder text. In that sense, the editor covers for edge case to ensure that the user can interact with these blocks even when they have no values set. What are the scenarios outlined above where Block Bindings should handle these attributes differently? |
I meant the former! Pardon if the wording was unclear.
This sounds right to me.
Thanks for outlining these cases. To Mario's point above, the primary scenario in which Block Bindings should handle this differently, at least as far as this issue is concerned, is the one in which a block is bound but the preview is unavailable for whatever reason. At the moment, those reasons can include the following when one creates a block binding (these are the scenarios I can identify, though there may be more):
Speaking of that last scenario, once the bootstrapping mechanism is merged, we should also be able to detect whether a source is valid on the client or not. That means we could potentially introduce an error state for the attributes panel if the specified source doesn't exist, an important part of the user experience which I think is also relevant to this discussion: |
I think we are having many related discussions at the same time. I'll try to gather the different scenarios to have the global context and I can try to split it into different conversations after that. Let's use custom fields as a real example to take a look at the different scenarios and the different possibilities. I'm sharing screenshots of how it works right now, but everything still requires a deeper discussion and changes may be needed. Uses casesUser can edit and the value of the custom field IS NOT emptyThis just works as expected. Nothing to decide here related to this discussion. User can edit and the value of the custom field IS emptyRight now, it shows the prompt text defined by each block. Some examples:
This doesn't seem right to me. Additionally, it seems that if I refresh the page, when a custom field has an empty string as a value, the editing gets locked. I think this is a bug. User can't edit the custom fieldWhen the user can't edit the custom field value, there are different use cases. The user can access the custom field value and it IS NOT empty The user can access the custom field value and it IS empty The user can't access the custom field value Right now, the editing gets locked and we show the value defined by the source as a fallback. In this case, it is the custom field key, but that needs to be revisited. Two questions come from this:
Custom field doesn't existThis is the same as if the user can't access the custom field value. It gets locked and shows the fallback we decide. Users editing a templateEditing a template is a different use case because there is no This is how it looks a movie template in the editor and in the frontend:
Again, this needs to be discussed and decided. Should it use the same fallback as the use cases where the custom field doesn't exist or the user can't access it? Aside from custom fields, we need to take into account that other sources will deal with similar issues. There are a couple of uses cases related to that. A block is connected to a source NOT defined.As the source is undefined, we can't show any value. What we are doing right now, is showing the original attribute value as a fallback and showing the source name in the attributes panel. A block is connected to a source defined ONLY in the server.In this case, right now we can't get the value dynamically in the client because the source hasn't defined what to do that. That means that we need to show some fallback. (See the possibilities section for the different alternatives). In this case, we could read the default content and the block attributes or let the source provide a fallback from the server. At this moment, we are showing the original value as a fallback like when the source is undefined, with the different that we will be able to access the label defined in the server after this pull request. Possibilities for fallbackAs mentioned in the previous use cases, there are scenarios where we can't show the value of a custom field (or other source), and we need to provide a fallback. These are some of the possibilities we have for that fallback. At this point, it is important to note that they are not exclusive. For example, we could use the fallback defined by the source and if it doesn't exist, fallback to the original attribute value. Use the original attribute value as a fallbackThis is what we are doing for undefined sources, for example. There are a couple of challenges with this approach:
Use the block placeholder as a fallbackThere is a block attribute named "placeholder" that is used in some blocks, we could try to reuse that somehow. I am not sure how this could work because the placeholder might change depending on the block attribute. Let sources define the fallbackRight now, we are using a
DiscussionsThese are the different discussions I get from the conversation so far:
More use cases not contemplated here? |
Thanks for outlining all of these use cases @SantosGuillamot! Overall, I think we need to acknowledge that many of these are distinct scenarios, and we should consider providing helpful information dependent on the situation so users can diagnose the state of the editor. I also agree with the general thrust of what @gziolo is saying. I’ll go through his points while also adding a few of my own suggestions that I think could help make this all more concrete.
I agree — we shouldn’t lock the field.
Yes, we should modify the prompt text. In terms of a placeholder, could we do something along the lines of, “Type here to modify {key}”?
Yes, we should indicate to the user what’s going on with the empty field. How about something like, “({Key} value is empty)” in gray text?
By default for all sources, I think we should say exactly what the situation is regarding the fallback, something along the lines of, “({Label} binding preview unavailable.)” I believe the label is the only user-friendly information that we’d be guaranteed to have from any source. Showing the default value doesn’t make sense — presumably that will get overridden and it’d be confusing to see one value in the editor and a different one on the frontend. The placeholder also doesn’t make sense, as it doesn’t indicate what’s actually happening. As far as letting sources decide the handling, what scenarios can we anticipate wherein the above message, “({Label} binding preview unavailable.)” would be insufficient? Either the preview is available or it isn’t. And if it isn’t, I imagine developers will have the best idea of what needs to be done in order to fix that (see my thoughts on the custom fields below as an example). So I lean towards allowing developers to define the fallback, since it seems an integral part of opening the editor API to them.
Yes, in my opinion the key on its own is too technical. If we’re in the post editor, I believe the custom field fallback would only be displayed after a block has successfully connected to the post meta source but is unable to retrieve the value using the specified key, essentially an error state. So we could say something like, “Post Meta binding unsuccessful. Please make sure {key} has been registered and is available to the REST API.” That’s a helpful message that could help developers and end users fix the issue. As far as other sources go, as mentioned in the point above, I'm not sure that we'd have enough information to display anything helpful to end users, besides that the preview is unavailable. If desired, developers could modify that message to be more suited for their specific circumstance.
I agree with Greg, let’s have an error state when the block is unable to connect to the source. Something I’ll add though is that in templates, when a block is successfully able to connect to a custom field via the post meta source, we should provide an indicator for the state, maybe just some curly braces around the key: “{key}”. I’m unsure how we should handle this scenario for custom sources — maybe creating another part of the API where developers can decide the treatment inside templates? Or perhaps that’s overkill. Maybe in that scenario, we just show empty curly braces or the connections icon.
This seems like a unique scenario worth handling differently. Can we say something like, “Unauthorized to see {label} binding preview”? I think there's already a lot to unpack here, so I can circle back to these following points as discussion continues:
|
Thanks a lot for the comments 🙂 I've been thinking about this and I'd like to offer a potential path forward to discuss. Let me know what you think. Before that, a few aspects to consider:
Potential solutionWith that context in mind, let's review the different scenarios and their possible outcomes. When the user CAN edit and the value IS NOT empty When the user CAN edit and the value IS empty I believe we can reuse the same message in both of them, at least as an initial step. I was thinking something like My main concern here is how it will look in blocks like Button. It feels a bit long, although it is true that it is only shown when the button text is connected to an empty field. We might need to look for alternatives for shorter text. When the value returned by the source is
My proposal is to start with Again, it feels a bit long. When the source doesn't exist In the future, we could include a Call To Action to show the UI to create bindings. Action itemsLet's discuss the overall proposal first, but I wanted to wrap up some action items if we decide to pursue this path. The specific details can be discussed in separate issues or pull requests. These are the items I've identified:
Once we agree on the next steps, I can include the different issues in the current iteration. Answering commentsWith this context, I'll go through the latest questions shared in the comments.
I don't think there is any reason and it should be considered a bug. It is included as part of the action items.
This sounds good and I believe it should be covered in the proposed solution.
I feel having just the source label could be confusing, especially in a template context. I can imagine having multiple blocks connected to different custom fields. Having "Post Meta" in all of them feels a bit weird to me. That's why I believe using "Post Meta - key" if the key exists feels better.
In my opinion having a "Post Meta - key is empty" message would work when the user can edit and when they can't.
For this initial version, I'm inclined to just show the "{sourceLabel} - {key}" message, at least until we learn more about what other sources need. It will help keeping consistency across the UI independently of the source used, and we won't expose APIs we aren't sure about. “({Label} binding preview unavailable.)” feels like an error message to me, which doesn't need to be the case. For example, in Post Meta, we return
I don't think it is an error state. For example, if the user is an "Editor" and doesn't have permissions to see that value, but the field exists for "Admin", it is not an error. It is just that the value is not accessible for the "Editor". Or if I am in a template where I don't have a post ID, it isn't an error either. I would try to avoid guessing what is happening, at least for the initial version.
If we start making custom solutions for Post Meta and we don't try to unify the UI across sources, we will end up with multiple APIs which could become really confusing.
Again, I'm not convinced about adding custom messages for each scenario. It doesn't seem to scale, especially with external sources. |
Thank you following up on my previous comment @artemiomorales and @SantosGuillamot. I think we are getting to the point that we have a good vision of what can be done as a first step without committing to any public API which is awesome. The next step would be to turn these ideas for placeholders (some changes discussed are in different PRs) into a working prototype where we can talk further the more delicate implementation details like labels that users will see. We can also change them later based on user feedback so I would prefer go with the version close to what Mario shared to speed up the exposure and the organic feedback loop. |
I've started some pull requests to address some of the mentioned points. I believe they can be managed independently and they add value on their own:
I plan to work now on adapting the fallback when the value from the source is not available (templates, for example) and remove the existing |
I've opened the last pull request to remove the With that, I believe we can keep the discussions in the different pull requests. Do you think we should close this issue? |
@artemiomorales and @cbravobernal, is there any follow-up work needed at the moment? I repurposed the issue as a regular task instead of the discussion as we arrived at multiple action items. |
I believe most of the points discussed in this issue have already been addressed. And I feel future improvements can be handled separately. I'm closing this based on that, but feel free to reopen if necessary. |
Context
With block bindings, we connect a block attribute with a dynamic source. However, the value of the dynamic source is not always available, so we should show a fallback value. For example, if I connect a paragraph with a custom field, that custom field might not exist, or I don't have permissions to see the value. Or when I am in a template and there isn't a specific post ID.
In those cases, we need to fallback to another value to not break the user experience. Simplified, the workflow could be understood this way:
Right now, there are two use cases in core:
key
as the fallback.I assume pattern overrides work as expected, but the fallback for custom fields feels a bit techy, so it'd be great to hear opinions if this is right or it should change.
The important part here is that the fallback might defer depending on the source. Right now, we added a
getPlaceholder
API to let sources decide what to show. This is how it is done for post meta, where we just return the key: link.However, it was done that way because it was simple and we knew the API was private. Now that we are considering opening the APIs, it's time to discuss the best approach here.
Potential approaches
These are the different approaches I was considering:
Option 1: Keep using
getPlaceholder
(orgetFallback
) callbackWe could keep the callback we are using right now. It provides flexibility because sources could do whatever they want but at the same time it makes the logic a bit more complex.
Option 2: Change the
getPlaceholder
property to be a string instead of a callbackThis would mean that the fallback would be always the same for each source. Imagine I define it as "Connected to Post Meta". It would show that message independently of which field the block attribute is connected to.
One potential benefit of this is that it could be defined from the server registration directly, and reused in the client.
Option 3: Use the
bindings
object in the block attributes to receive thefallback
valueRight now, bindings are defined inside the
metadata
attribute. I suggest adding a new property in the bindings attribute that receives the fallback. Something like this:In this case, the UI would be in charge of adding that property. For example, when a user connects a block attribute with a custom field in the upcoming UI, it could automatically insert the fallback the same way it's adding the rest of the object.
Option 4: Don't use a placeholder at all
This would mean skipping the placeholder step and always fallback to the original value of the block attribute when the value returned by the source is undefined. This is how pattern overrides work right now.
If I understood it correctly, this is what @richtabor was suggesting in this comment. Please correct me if that's not the case.
I'd love to know any thoughts on this before making a decision 🙂
The text was updated successfully, but these errors were encountered: