-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Make it possible to push local block styles to global block styles #44361
Comments
Love this idea! |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
I don't think it would be hard to do that.. I tried something real quick and it seems it will be fine. --cc @ciampo as he's a Popover expert right now 😄 . |
This comment was marked as resolved.
This comment was marked as resolved.
That looks like a nested dropdown menu, which to my knowledge we don't currently support (or at least I think! I don't remember seeing a nested dropdown menu in gutenberg before). The proper way to implement that would be extend the functionality of |
This comment was marked as resolved.
This comment was marked as resolved.
While we didn't "blanket dismiss it" above, I just wanted to reiterate the fact that nested dropdown menus can be complex to implement correctly, especially in terms of accessibility, pointer and keyboard interaction. It would probably wise to also think about an alternative UI that doesn't employ nested dropdown menus, in case anything comes up. Another couple of things to keep in mind that may be important to consider:
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
I hear that, and I think there's sense in exploring an inspector affordance for it which considers the incoming tabbed interface. However it is also an advanced feature. In Figma, for example, I use it all the time, but the feature is incredibly buried under an obtuse Edit > Copy properties menu item, or by clicking a very tiny hit area in the fill-layers and pressing ⌘C, then ⌘V on another object. I like to think that it's important to find the right balance here as well, one where it's ergonomic and easy for a theme developer to routinely use the feature, but still managing its prominence. If the flyout submenu had shortcut keys visualized in the menu, would that help thread the needle? |
I gamble it'll go from an advanced feature to a ubiquitous tool for everyday builders to leverage as part of their site design/building, but merely speculating 🔮 . For now, I'm fine with it hidden away, but would not be surprised if it is necessary to revisit the user interface further down the road 👍 |
This comment was marked as resolved.
This comment was marked as resolved.
@jasmussen @javierarce this is some really nice work. I'm a fan of the Advanced sidebar integration and love the "dirty" indicator flow. I just had a small suggestion and please excuse the 5-minute design modification in Preview app, but thought it might be good to try and reinforce which styles items are dirty in the sidebar once they're modified and used @jasmussen latest image and modified it quickly. I'm not entirely happy with the circle colors/treatment and was mostly just going for a quick visual to demonstrate my idea. |
I think that's a nice idea @colorful-tones, and could facilitate per-panel-pushing and resetting. The latter will be particularly important if you've extensively modified a block and want to push everything except your border changes (as an example). |
I'll remember to share the Figma links more often — thanks so much for the added exploration! Conceptually I like the idea of per-panel style pushing, but I'm a bit concerned with unread-dot fatigue. When I see such a dot, I want to address the issue it's pointing me to — which is why it's good for the multi entity saving interface, and personally frustrating for an interface where I can't make it go away, such as in the MacOS preferences: I'm not using iCloud. In this case, I might be confused at dots appearing for local changes I made. However, and to expand on Jay's point above, there might be an opportunity to explore embedding this feature inside the panel ellipsis menu, this one: Not sure how it would look, but there would potentially be room for a section similar to what we put in the Advanced section for now. What do you think? |
I think it makes a lot of sense and would make this feature much more flexible. The menu item can probably be the same as the button you have in the Advanced panel: "Push changes to global styles". We could include the panel name "Push dimensions changes to global styles", but it's probably not necessary. That said, I do think it's fine to have a whole-panel push affordance to begin with, and add the per-panel pushing as a follow up. Like you said, the dot isn't perfect and there are other options we can explore to indicate when a panel has departed from global styles. |
Cool, I'll update this issue with the newest mockups, but point to the comments that discuss potential followups. |
This issue has been updated with mockups, and is ready for a dev to pick it up! |
For any dev picking this up, be sure to also see #44418. |
I'm having a look at this for now until it gets too hard or I run out of time 😄 |
Great point. I was just thinking about this and I believe the caveat must be that only styles available to global styles are valid. For example, see the Cover block global styles here: It does not include the custom controls in the Cover block for min height and overlay background, since these are block attributes not styles that can be defined in theme.json. Also, we need to think about how to deal with presets, which are usually stored in the block's top-level attributes, and not in the style object. E.g., {
"fontSize": "a-font-slug",
"styles": {
"typography": { ... },
...
}
} It might be easier to communicate this constraint in the sidebar for this issue, but trickier for copy/paste functionality. Edit: for copy/paste maybe we could limit it to block supports styles, e.g., typography, dimensions, color... 🤷 |
I've been looking into this (rather slowly as I've had to dive down a few unfamiliar rabbit holes), and am just jotting down some notes. I think the first version should target the site editor only. This is because a Furthermore, in the context of the site editor, there might be less confusion about which styles are being pushed. Many blocks have custom "style" attributes, e.g., overly color for the Cover block, column count for the Columns block, which are not compatible with theme.json/global styles. Also, and probably an esoteric point, we'll want to make sure that we don't overwrite any other style properties when updating an individual property. For example, if we change the top border color for a Group block, we want to make sure we're only pushing that one value globally, and nothing else. Another thought I had was about block attributes themselves. If a block's styles are pushed globally, should that block retain the styles at the block level? My instinct is to say "yes" to avoid unforeseen styling issues if global styles are cleared. Just a thought I had. Down the line, we might want to leave the door open to allowing individual style properties to be pushed. So a user might only want their changes to #45961 is a very crude prototype, and I'm not sure I'll have enough time to get something acceptable working for the purposes of Phase 2 in the next couple of weeks. If it comes down to that I'll unassign myself and add any learnings here. P.S. There might be a few things we can learn from #34178, or maybe not. The aim of that PR was to allow blocks to access global styles. Just noting here so I don't forget 😄 |
In a recent hallway hangout with the FSE Outreach Program, we discussed this issue alongside #44412. How will these two interconnect, meaning if I set up custom CSS and try to push globally, how will that be handled? |
Thanks for the question; it's a good one. I believe, based only on what I've learned so far in tinkering with #45961, that the mechanics will be slightly different. To push custom CSS globally, I suspect we'd have to parse the custom CSS for the limited style properties and sub properties that global styles supports - typography, color, border and so on. cc @glendaviesnz for fact check Saving them to the user styles entity via the global style context will be the same I'm assuming. |
After discussing with @noisysocks I've closed #45961 (comment) with comments. A follow up PR will be available at some point soon - one that will be, I suspect, less complex 😄 A few further things we'll need to keep in mind that came out of the discussion:
|
I could be missing something, but what would be the use case of not pushing all styles?
Can you elaborate? Is this because you are moving the local styles to being global? If so, that seems valid enough only after you actually save those custom styles.
Yes. Question, would this undo persist if you save the global styles? And in that case, would the undo also undo on the global level?
I'm having a hard time parsing the example. If a block's local styles are reset, what would they then source from? And wouldn't those styles sourced, if pushed, then be the new standard, saved in the user style? |
Sorry, end of day brain dump without rereading 😄
The only case I could come up with was when a user wants some local block styles, but not all of them, pushed. For example, I've made some changes to a local block, and I then return tomorrow and make some more. I only want the second round of changes I make in real-time to apply globally. It's a contrived example, and I do think it's totally okay to push them all at once, but I wanted to raise it to indicate that we had at least considered it. Here's an alternative PR that pushes all styles: #46446 (kudos to @noisysocks for working on that)
The assumption is that local block styles that have been pushed globally no longer need to be applied to the local block. So after the local styles are pushed, we can delete them from the local block styles, since they'll be inherited from global styles. There is a downside to this: if a user resets global styles, expecting that the local block styles will persist. If we remove them when pushing to global styles, they'll be gone forever. cc @noisysocks
I think so, yes. We're currently looking into it. @noisysocks started investigating yesterday, but the work of undo is voodoo so we might have to come back to you once we've verified how it will and how it can work.
Sorry, it is a bit strangely worded. I tied myself in a knot over this one, and I believe it's not a valid concern. So you can ignore me. To explain, I was pondering over the following scenario:
So I was asking myself what "reset" meant here and whether any user global styles also had to be "reset" back to what they were. But now I think about it, the point seems invalid: if I reset a local block's styles and then push globally, I'd expect that all blocks of that type will look the same as the current block, that is, with reset styles. |
When editing a block in the post or site editor, it would be nice if there was a way to push the styles for that block to global styles so that all blocks across the site are updated. It could work like this:
The button in "Advanced" adds the "unsaved changes" dot:
That means publishing now surfaces an option to "publish" the styles that were pushed:
I still think the nested menus option would be the clearest path forward, which would manage prominence. Using the option here would also add the "unsaved dot" and require multi entity saving:
A smaller option would be to accept a longer ellipsis menu:
As a summary:
Issue updated Nov 9. Props @javierarce for mockups.
The text was updated successfully, but these errors were encountered: