Move Entities Saved States from Modal to Sidebar #21522
Conversation
|
Size Change: +9.19 kB (1%) Total Size: 825 kB
ℹ️ View Unchanged
|
Addison-Stavlo
requested review from
ajitbohra,
chrisvanpatten,
mkaz and
youknowriad
as code owners
There was a problem hiding this comment.
Feature wise, this makes sense and I'd love to land an iteration of this but from the code perspective, I have some concerns, the main ones are:
We shouldn't be relying on the "editor" package ini "edit-site". The editor package is a post-specific one. We might need some refactoring to achieve this though.
The "edit-post" package also has the concept of "pre-publish" panel, I'm not sure we can share code (at least as is) with the "edit-site" package since the logic for that panel is buried somewhere in the editor package, but they should at lease behave closely. I wonder if there are some generic components to extract while leaving the specifics to each top level package (edit-*).
|
@youknowriad thanks for taking a look!
Ah I see!
Thats essentially what this is modeled after (mostly from the scss) with a +1 on the z-index so it plays well with the publish panel inside the post editor as well. |
|
Just noticed that "EntitiesSavedState" is on Editor which doesn't make a lot of sense to me. I think it should probably be in "core-data" since it's related to "entities". That said I'm fine with keeping it there in this PR and solving that part separately. What I'd recommend is to try to build this PR without introducing any editor specific things (core/editor store or components) into EntitiesSavedState, potentially, by using props for the component. It's going to be a challenge to try to reuse things between edit-site and edit-post. block-editor is not going to be the right place for these when they deal with WordPress specifics and APIs. |
It looks like the only Then our edit-site would call core-data instead of the editor package and we wouldn't be conflicted there. And the edit-post package would continue to call editor for editPost/savePost where it does and would move to using core-data for the saved states sidebar. Does the above sound good? Or is there some other conflicting piece I am missing with this? |
We shouldn't move UI related state to core-data. UI related things should stay in the UI related packages (edit-post, edit-site). I think we should use props. |
Gotcha. For the Modal, we were using props and passing down to Modeling functionality around how the publish panel works, instead of |
I think that's totally fine, we shouldn't be afraid of duplicating code. Sometimes I feel early abstractions do more harm than good. Now later, if we want to reuse more stuff, maybe the "interface" package is a good fit since it's a "WPAdmin UI" package but I'm still not sure about the APIs at this point so better to start simple. |
|
@youknowriad - That makes sense to me. I removed the store stuff from the editor package and moved them to edit-post and edit-site instead. The After taking a quick look at moving
Maybe "interface" would make sense as the home for both the saved states component and its associated store in the end? 🤔 Once we are more sure about the APIs and want to consolidate/refactor this around more. |
|
I added some updates on the post-editor side so that:
And on the site-editor side, I added the "open save panel" a11y button. |
|
Just noting that I'm not be able to review this on the next couple days. @epiqueras you might be interested in this PR too. |
Co-Authored-By: Enrique Piqueras <[email protected]>
|
This is so sweet. It would be cool to see reusable blocks move to a similar saving approach. Nice work here, can't wait to use it! 🙌 |
Conveniently, #21368 sets us up for something like that 👀 |
There was a problem hiding this comment.
Let's do it! 
I noticed a thing where an edit to a template part after an initial save did not make the editor think that the TP had changes. so it doesn't persist the change to the DB when you click save. I'm going to assume that this will be fixed by #21368, since we already know that dirty states aren't being computed correctly. 😁 But I thought I'd point it out because I didn't think this was a bug on master
@noahtallen, I'm not able to reproduce this. I tried both inserting an existing TP as well as creating a new one. The TP always seems to come up as 'dirty' after changes are made to it on my end. I've tried edits to them w/ w/o / before / after an initial save. Maybe I am not going through the same flow? |
|
no worries :) I don't think it should affect this PR then. If it comes up again, it would probably be related to the code that I'm working on anyways rather than the UI aspect of displaying it |
|
I think all the requested changes have been addressed. Are we in a good state to move forward with this now? |
There was a problem hiding this comment.
This worked as expected in my testing. Nice work @Addison-Stavlo! 🎉
I noticed a minor issue with Select functionality. Clicking it for the second time will de-select the template part, and consecutive clicks won't toggle it back. However I don't think it's a blocker and it can be addressed in follow-up PR if needed.
| font-size: 18px; | ||
| margin: 20px 0 10px; | ||
| .entities-saved-states__panel { | ||
| @include reset; |
There was a problem hiding this comment.
Do you recall why we need a "reset" here. Seems weird since this is meant for global resets.
There was a problem hiding this comment.
Ideally, components don't depend on global resets though.
There was a problem hiding this comment.
It may not be necessary. I think I added it b/c post-publish-panel was using it at the time as well and I was trying to ensure the styles were the same between the two. It looks like it's not there anymore either.
Description
This PR explores moving the
entitiy-saved-statescomponent from a Modal to a Sidebar as discussed in issue #20421 .This also enables applicable entities to be selected in the editor through the sidebar.
Notes on design parity (or lack thereof) and future goals:
How has this been tested?
Tested on local docker setup in the various post editors and site editor.
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: