Create classic navigation menus as draft and dirty the site editor #43580
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cbravobernal
added
[Block] Navigation
getdave
reviewed
Aug 24, 2022
packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
Outdated
Show resolved
Hide resolved
packages/block-library/src/navigation/edit/use-create-navigation-menu.js
Show resolved
Hide resolved
|
I tested this and found one issue:
Also following the steps above, then reload the editor, I see this: |
|
Size Change: +204 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
|
I added a commit to address the backend part of the issue I mentioned. Rather than simply returning an empty string if the ref post is a draft, we should allow the normal fallback to happen. We still need to address this in the site editor though. |
|
I added a commit to address the issue of draft navigation posts not being shown on the selector. I would love to have some feedback if this would be a correct approach and also if it's better to move |
draganescu
reviewed
Aug 25, 2022
There was a problem hiding this comment.
This PR is doing a good thing, but it feels like we're fighting with the system.
I think the intention to fix unexpected changes to the live website is great but ti would be lovely if we could approach the entity system in the editor to allow us to handle this better. To me it feels like we complicating even more what is already a very complicated block.
Any new entity should be created as draft and only set to status publish when user saves via the entity save flow. There should be a way for users to manage these drafts. Draft entities should persist in autosaves.
In my view, the disconnect between save and publish is a problem in the whole editor and the navigation block is probably the one where this is causing the most obvious problems.
|
Thanks for the feedback @draganescu !
We "auto set to publish" without saving the entity just to make the editor dirty in order not to be blocked, but if there is a cleaner option, I will be happy to apply it! (My knowledge about this matter is not the best one 😓 )
I completely agree with this. |
cbravobernal
force-pushed
the
try/fix-classic-menu-creation-without-saving
branch
from
ba29688 to
ee19472
Compare
draganescu
reviewed
Aug 31, 2022
|
|
||
| if ( ! navigationStorage.getItem( 'nav_menus_created' ) ) { | ||
| navigationStorage.setItem( | ||
| 'nav_menus_created', |
There was a problem hiding this comment.
I think this is more like classic_menus_imported unless I am reading the code wrong?
There was a problem hiding this comment.
Yep, I thought about similar cases that could happen with block menus, so put the name for every type. But seems this only will happen with classic ones.
draganescu
reviewed
Aug 31, 2022
packages/block-library/src/navigation/edit/navigation-menu-selector.js
Outdated
Show resolved
Hide resolved
draganescu
reviewed
Aug 31, 2022
packages/block-library/src/navigation/edit/navigation-menu-selector.js
Outdated
Show resolved
Hide resolved
draganescu
reviewed
Aug 31, 2022
There was a problem hiding this comment.
Excellent work so far, particularly disabling re-importing, adds a perception of stability to this block :)
I think this behavior makes sense:
- I have TT1 blocks
- I have no block menus
- I see the page list fallback
- I import a classic menu
- I click Save
- I deselect the checkbox under Navigation Menus
- I click save
- The post is still dirty
- I don't see the imported classic menu on the front end
- I reload, the imported classic menu is still there
- I save
- I see the imported classic menu on the front end.
I tested when having block menus and it also worked well.
getdave
reviewed
Sep 1, 2022
Comment on lines
82
to
88
| /** | ||
| * Immediately trigger editEntityRecord to change the wp_navigation post status to 'publish'. | ||
| * This status change causes the menu to be displayed in the frontend and set the post state as being "dirty". | ||
| * The problem being solved is if saveEditedEntityRecord was used here, the menu would be updated on the frontend and the editor _automatically_, | ||
| * without user interaction. | ||
| * If the user abandons the site editor without saving, there would still be a wp_navigation post created as draft. | ||
| */ |
There was a problem hiding this comment.
Suggested change
| /** | |
| * Immediately trigger editEntityRecord to change the wp_navigation post status to 'publish'. | |
| * This status change causes the menu to be displayed in the frontend and set the post state as being "dirty". | |
| * The problem being solved is if saveEditedEntityRecord was used here, the menu would be updated on the frontend and the editor _automatically_, | |
| * without user interaction. | |
| * If the user abandons the site editor without saving, there would still be a wp_navigation post created as draft. | |
| */ | |
| /** | |
| * Immediately trigger editEntityRecord to change the wp_navigation post status to 'publish'. | |
| * This status change causes the menu to be displayed on the front of the site and sets the post state to be "dirty". | |
| * The problem being solved is if saveEditedEntityRecord was used here, the menu would be updated on the frontend and the editor _automatically_, | |
| * without user interaction. | |
| * If the user abandons the site editor without saving, there would still be a wp_navigation post created as draft. | |
| */ |
Comment on lines
110
to
112
| const importedClassicMenus = new Map( | ||
| JSON.parse( window.localStorage.getItem( 'classic_menus_imported' ) ) | ||
| ); |
There was a problem hiding this comment.
Will this now run on every render. Is that desirable? Should we have it memoized?
Comment on lines
662
to
673
| importedClassicMenus.set( | ||
| navMenu.id, | ||
| classicMenu.id | ||
| ); | ||
| window.localStorage.setItem( | ||
| 'classic_menus_imported', | ||
| JSON.stringify( | ||
| Array.from( | ||
| importedClassicMenus.entries() | ||
| ) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
This pattern is fairly "noisy" as it's repeated in a simpler form in various places. I wonder whether we can create a simple, well-named abstraction for the process?
Comment on lines
45
to
58
| const importedClassicMenus = new Map( | ||
| JSON.parse( window.localStorage.getItem( 'classic_menus_imported' ) ) | ||
| ); |
There was a problem hiding this comment.
This appears to be the same as what we do in the main Navigation edit file. Can we pass a prop into this component?
| @@ -100,8 +107,13 @@ function Navigation( { | |||
| setAttributes( { ref: postId } ); | |||
| }; | |||
|
|
|||
| const importedClassicMenus = new Map( | |||
There was a problem hiding this comment.
Thinking in general about whether these local storage interactions could ultimately be pulled into a custom hook?
There was a problem hiding this comment.
In WooCommerce Blocks repository there is something similar.
cbravobernal
commented
Sep 1, 2022
packages/block-library/src/navigation/edit/navigation-menu-selector.js
Outdated
Show resolved
Hide resolved
|
I've taken this for another spin and it is looking really great. Thanks for all your work on it. |
cbravobernal
commented
Sep 2, 2022
Comment on lines
1
to
23
| function useImportedClassicMenus() { | ||
| if ( ! window.localStorage.getItem( 'classic_menus_imported' ) ) { | ||
| window.localStorage.setItem( | ||
| 'classic_menus_imported', | ||
| JSON.stringify( [ ...new Map() ] ) | ||
| ); | ||
| } | ||
| const importedClassicMenus = new Map( | ||
| JSON.parse( window.localStorage.getItem( 'classic_menus_imported' ) ) | ||
| ); | ||
|
|
||
| function setImportedClassicMenu( navMenuId, classicMenuId ) { | ||
| importedClassicMenus.set( navMenuId, classicMenuId ); | ||
| window.localStorage.setItem( | ||
| 'classic_menus_imported', | ||
| JSON.stringify( Array.from( importedClassicMenus.entries() ) ) | ||
| ); | ||
| } | ||
|
|
||
| return { importedClassicMenus, setImportedClassicMenu }; | ||
| } | ||
|
|
||
| export default useImportedClassicMenus; |
There was a problem hiding this comment.
As I'm not 100% sure of using localStorage as a solution, I will remove this and wait for having slug as references to try it.
cbravobernal
force-pushed
the
try/fix-classic-menu-creation-without-saving
branch
from
4d83ab7 to
c9c4d2f
Compare
|
Right now we can have two different approaches to this PR. After importing a classic menu, if the user reloads without saving:
I will try to work on getting the editor using the fallback on the first point, as the UX is better than in the second one. |
|
@c4rl0sbr4v0 IMO either of those solutions is better than the current experience, so we might want to merge this in a working state (without local storage) and then return to the question of what happens when you refresh the editor in a followup PR |
|
@scruffian I added the code to not use draft menus as a fallback. Now it should work fine. |
scruffian
approved these changes
Sep 5, 2022
cbravobernal
force-pushed
the
try/fix-classic-menu-creation-without-saving
branch
from
2797061 to
130a93e
Compare
cbravobernal
deleted the
try/fix-classic-menu-creation-without-saving
branch
45 tasks
82 tasks
This was referenced May 4, 2023
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
It may fix #42850
Why?
Right now, if a user creates a navigation menu on the site editor and then, without saving, abandons the editor. The menu appears as created and empty. Having an empty menu causes:
If the user, instead of creating a navigation one, selects one existing classic, and then without saving, abandons the editor. The menu appears as created and with the content of the classic menu. This causes:
How?
With this PR, what we do, is set the classic menu auto-created as a draft, preventing the previous causes. If the user closes without saving, it will:
Caveats
Edit: The caveat has been kind of solved, by user, saving into local storage the Classic Menus imported so they disappear from the menu list of Classic Menus and are only at the top for selection.
Second edit: I'm not sure about using local storage for this solution, so decided to remove it and wait for the slug PR
Testing Instructions and screencast
I did a video with the testing instructions and a quick summary (better watch it in x1.5)
https://www.loom.com/share/fad5b6be9fe8421c88fe4f3a7f56f41e