Refine rename flow for blocks with overrides #60234
Conversation
kevin940726
added
[Type] Enhancement
|
Size Change: +721 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
Nice work, although there are some tweaks to be done here and there, this seems to have the right ingredients. Can you have a look at this one and see if that helps thread the needle? There may be some simplified name management by unifying the modal. |
There was a problem hiding this comment.
This works pretty well. I left a few PR comments, mainly about the copy which could do with some adjustments.
In testing, there's one flow that doesn't seem to be covered:
- Create a pattern
- Add a paragraph
- Use the block options dropdown menu (either via List View or the canvas) to name a block.
You see a different UI in the modal when doing this, but still have overrides enabled after naming the block. Feels like there might be two options to solve this:
- Show 'Allow overrides' in the dropdown instead of 'Rename', and use the same modal in all places (when editing a pattern). That way everywhere has the same flow.
- (Or) allow the user to name block as normal, but don't enable overrides. Instead, still require the user to click 'Allow overrides' in the advanced area.
In practice, the 'Allow overrides' button that turns into a toggle also doesn't feel like the best option to me, but it seems more of a minor thing that can alway be iterated on.
|
Took this one for a spin, and I'm missing a little nuance, seeing the toggle but no block name most of the time, and I wasn't able to find the naming button. It's very probably I'm doing something wrong. I would appreciate a quick review by @SaxonF |
Yeah, this is based on the assumption that naming a block automatically enables overrides. You need to "opt-out" of the feature manually after naming it (#59812). (I have multiple explorative PRs all connecting together, and I know this is kind of confusing 😅)
The naming control in the advanced panel is removed by design. This also matches the proposal in #60118. This is just one possible option though and we can revisit this separately. If you can't find the rename button in the dropdown then it's likely a bug though 😅. |
I would not automatically enable syncing via renaming, at least initially. The direction this PR goes in is adding an explicit way to enable overrides (vs based on name) whilst encouraging a name change, which I think is the right approach at least in the short term. We keep the renaming action and overrides action separate, but can tie them together via the "allow overrides" button. What do we need to do to get this over the line? I think its a big improvement over what we have and is a sensible start point. Keen to get @fabiankaegy thoughts on this too. |
|
Just to flag some dependencies. I think this design direction (opt-in to block naming) has a knock on effect with this PR - Use metadata.name only as the hint for pattern overrides. I think it makes it unworkable as a solution, so it'd mean going back to the drawing board on a solution for the issue that it would have solved - Pattern Overrides: individual binding attributes are saved in the pattern markup and don't auto-update. Does that conclusion sound right, @kevin940726? For me, it's not a blocker, but I want to be clear about what the interconnected tasks are. |
|
Thanks for the ping :) This tests well! Kudos to everyone involved. I still stand by my strong opinion that the overrides toggle should not be enabled by default when a block gets renamed and instead decouple that action so that a user has to specifically opt-in. Besides that I noticed a few rough edges:
|
kevin940726
changed the base branch from
update/pattern-overrides-opt-out
to
trunk
kevin940726
force-pushed
the
update/pattern-overrides-rename-ui
branch
2 times, most recently
from
d48f797
to
5e8be30
Compare
@fabiankaegy The block name was removed completely from the advanced panel so not just when override is enabled. See #60118 (comment) . There are a couple reasons for this that the issue touches on but the one related to this work is that renaming has the side effect of resetting instances that already override the block. This is a destructive action which is hard to provide guardrails around if its simply an input within the inspector with no confirm action. The other main purpose of this exploration is clearly separating "rename" and "allow overrides" actions, hence not including toggle within the rename modal. Rename modal can be consistent whether its used in the source or on a random block in the editor. Relabelling the initial "allow overrides" button in the inspector to "set up overrides..." which triggers the "set up overrides" modal may help clarify why it turns to a toggle. |
|
@SaxonF thanks for those insights. I wonder whether completely disabling the rename field may be a better solution. Removing it makes it seem like there is a bug |
|
I updated the branch to add an "Allow overrides" toggle in the renaming modal when editing inside a pattern. I also made the toggle "opt-in" instead of opt-out to avoid some of the issues mentioned above. Kapture.2024-04-03.at.18.47.52.mp4Note The code for pattern overrides probably shouldn't belong in the |
|
@fabiankaegy the name field shouldn't be there at all , are you seeing it visible on this branch in some cases? Consider removing block name from advanced a completely separate issue that also benefits this work . |
|
@kevin940726 as noted above, I'm not convinced we need the overrides toggle in the rename modal and would prefer to keep rename and overrides separate actions for now. If we get feedback that people are looking for overrides in rename or related we can reassess. |
The name field is visible in the advanced section of any block as of WordPress 6.5. I think that this ship has sailed. I would be very careful about removing it again from there because users are now getting used to it and I think it would be quite disruptive to remove it again. |
|
Just chatted with Saxon about the feedback here, to better understand some of the path being explored in this PR. The short summary revolves around these two points:
I'll dive in with more details from that chat, but do it over a quick visual of the current state of this branch, with some points intermixed as feedback. GIF: The GIF shows editing a synced pattern, selecting an image, then clicking the "Allow overrides" button. This button is meant to be a "setup" feature for setting up overrides. The motivation here is that allowing parts of the pattern is a bit of an advanced flow, so the modal can provide contet. But as soon as a user-supplied name is provided, then the "allow overrides" just becomes a toggle. Separately I try to rename a group block, just in the list view — turns out there's a bug there, the name for the group block is not reflected in the inspector. Naming Unpacking this — the block needs a name when overridden. Moving this into the modal invoked by the "Allow overrides" button makes it possible to provide context for why it needs a name. There's also just the ergonimics of renaming a group block, without enabling any overrides. This is a purely cosmetic change that is visible primarily in the list view. Here's an example of the 6.5 release site structure:
Every module in that page is a Group block, and with a lot of content to build and edit, those cosmetics provide a meaingful overview and navigational affordance for anyone editing. But only really, in the list view. And since that's also the primary interface for renaming (ellipsis menu > rename), the fact that you can rename also in the Advanced section, is a little duplicative. To that end, I can get behind the idea of removing the block name from the advanced section, and having that ellipsis menu be the primary place where you do that. So I can be on board with this PR as a step forward: having the button to initially provide a name for overrides makes sense. Removing the block name from advanced, also makes sense. There may be room to revisit the toggle that appears afterwards could be something to explore, but as-is it does provide a simple way to at a glance tell whether something is overridable or not, so it seems worth trying. Next steps Based on the above, I'd suggest the following. Firstly, to fix the bug where you can't rename a group block — not sure why that broke. Separately, let's massage these modals:
Those last two are the same modal, the latter just has a contextual warning for when the the block allows overrides. Let's remove the checkbox and make them look more alike. A suggestion for the above three: 
|
|
Thanks @jasmussen and @SaxonF :) That overview is very helpful. I stand by my comment from a moment ago in that I think it would be a wiser choice to disable the name field in the advanced section when overrides are active and maybe trigger the modal when a user attempts to click on it instead of removing it altogether. Besides that one detail, I really think this flow that you are outlining is going to work very well :) |
|
@fabiankaegy i don't see removing name input as a blocker for this work so could just provide warning text under input in short term and discuss name field in Rich's separate issue. |
|
Also added a comment to the ticket you mentioned #60118 (comment) I think taking it out of this PR and then seeing whether we can come up with an alternative before we remove it altogether would be better. But I won't block progress here :) We have some time before the next WP core release to get this right |
Agreed.
We shouldn't trigger a model on input selection. And I also don't think having an input that is sometimes there, isn't ideal. I do think the Advanced panel is more confusing having the name input there, where it's largely irrelevant (you rename for List View). But let's carry on that conversation in #60118 |
|
@richtabor i wonder whether it should have the purple color at all. Because the purple is always meant to represent global elements. And the override is exactly the opposite 🤔😅 |
|
We can certainly iterate from here, but I did note that it's unexpected to have two different interactions for enabling and disabling overrides, and especially two different interactions for enabling. Can we simplify to this below (don't mind the actual design metrics, but the flow): CleanShot.2024-04-10.at.12.31.44.mp4Removing the connection between I click "Allow overrides" and a modal renders with the name field. I am required to add a name if there is not one. If one already exists, I see it in the input field already. I select "Allow overrides" to confirm my override. When I press "Allow overrides" on the modal, the binding is applied—just like when I first created a binding initially. Now I see the secondary button in the Inspector, but it renders "Disable overrides". When I select that button, I see a "Disable overrides" modal to confirm my decision. The help text should state what will happen to my existing overrides. There is no name field on this modal. When I press "Disable overrides" on the modal, the binding is removed. |
Yea, perhaps that doesn't work 1:1. Thoughts on using purple for bindings/overrides @jameskoster? I suppose this is where bindings and overrides gets muddied a bit. As blocks with bounded attributes have values derived from "elsewhere"—whether or not that's "global" is another question. |
|
Agree it's not perfect, but on balance I don't mind it too much because parts of the block are still synced (styles). Worth noting perhaps that overrideable layers in Figma's component instances are always purple. That said, maybe the icon remains purple until there's an override, and then switches to the regular color. Could be worth a try? I worry it will be tricky to keep track of all this color changing though, across list view, inspector panel(s), etc. |
|
@richtabor if we feel the button communicates both state and action (enabled and disable) then I'm fine with that approach. |
|
Using colour only to indicate whether a block has been overridden will be an accessibility issue. If we can establish a common pattern we can use for styles (locally set vs global) and blocks that would be preferable. Eg a dot to indicate a change |
I think it's fine yea. |
kevin940726
force-pushed
the
update/pattern-overrides-rename-ui
branch
from
bb54d78
to
934c753
Compare
This is fixed in |
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
kevin940726
force-pushed
the
update/pattern-overrides-rename-ui
branch
from
677a29d
to
37b75a3
Compare
What?
Part of #59813. Supersedes #60066. Also close #60118.
The latest design is #60234 (comment)
This PR does a few things:
Why?
This is to simplify the UI flow for renaming and toggling overrides for blocks.
How?
Add overrides warning to the rename modal and change the overrides control to a button when there's no name.
Testing Instructions
Screenshots or screencast
Kapture.2024-04-03.at.21.34.10.mp4