Refactor classic menu conversion process #38858
Merged
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
8 tasks
|
Size Change: +946 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
adamziel
reviewed
Feb 17, 2022
|
|
||
| const safeDispatch = useSafeDispatch( dispatch ); | ||
|
|
||
| async function convertClassicMenuToBlockMenu( menuId, menuName ) { |
There was a problem hiding this comment.
I'd pull it out of the hook, there's no need to construct a new function on each run and it would be more readable to me (more separation).
There was a problem hiding this comment.
I take your point but if you follow the flow you'll notice that this is required to be within a hook because it uses other hooks. In fact it's hooks within hooks within hooks.
I'm not in favour of this approach really but I don't think we should look to refactor in this PR because it will become overly complex to review.
How would you feel about pairing on a refactor of this sometime?
There was a problem hiding this comment.
Sure, let's try to pair up tomorrow
There was a problem hiding this comment.
With the recent updates in place I feel pretty good about having this here, let's just wrap it in useCallback.
adamziel
reviewed
Feb 17, 2022
packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
Outdated
Show resolved
Hide resolved
adamziel
reviewed
Feb 17, 2022
|
|
||
| const convert = useCallback( | ||
| ( menuId, menuName ) => { | ||
| if ( ! menuId || ! menuName ) { |
There was a problem hiding this comment.
You can have an async function inside useCallback, only you have to declare it separately, e.g.:
const convert = useCallback(
( menuId, menuName ) => {
async function doTheWork() {};
doTheWork();
}
);There was a problem hiding this comment.
Yeh but in this case the aim isn't to return Promise from convert(). We want to run a routine and then have that routine update the state which we can then use outside of the hook to decide on various things such as loading states.
This relieves the hook consumer from having to reimplementing the isFetching, isError...etc inside their component. Rather all this is encapsulated and we simply expose a state which the consumer can use to decide on side effects within a useEffect.
There was a problem hiding this comment.
All I'm saying is there are two code styles available to choose from. Note that my snippet above doesn't return a promise either.
There was a problem hiding this comment.
Oh I see. You're just saying we could move the async function outside of the useCallback and that would be ok.
Unless it's a blocker for you, I'd rather not do that now because I'm planning a follow up that might change things a little. I ping you when that's up.
adamziel
reviewed
Feb 17, 2022
packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
Outdated
Show resolved
Hide resolved
This was referenced Feb 18, 2022
This comment was marked as outdated.
This comment was marked as outdated.
getdave
force-pushed
the
try/refactor-classic-menu-conversion-process
branch
from
7a195c5 to
b0a7d1d
Compare
getdave
commented
Mar 3, 2022
getdave
commented
Mar 3, 2022
getdave
commented
Mar 3, 2022
| // - we don't have uncontrolled blocks. | ||
| // - (legacy) we have a Navigation Area without a ref attribute pointing to a Navigation Post. | ||
| const isPlaceholder = | ||
| ! ref && ( ! hasUncontrolledInnerBlocks || isWithinUnassignedArea ); | ||
| ! ref && | ||
| ! isConvertingClassicMenu && |
There was a problem hiding this comment.
No showing a placeholder if we're running the conversion process.
getdave
commented
Mar 3, 2022
| // - there is a ref attribute pointing to a Navigation Post | ||
| // - the Navigation Post isn't available (hasn't resolved) yet. | ||
| const isLoading = !! ( ref && ! isEntityAvailable ); | ||
| const isLoading = | ||
| isConvertingClassicMenu || |
There was a problem hiding this comment.
Now loading UI state should show immediately if we are running the conversion.
adamziel
reviewed
Mar 4, 2022
packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
Outdated
Show resolved
Hide resolved
adamziel
reviewed
Mar 4, 2022
packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
Outdated
Show resolved
Hide resolved
adamziel
reviewed
Mar 4, 2022
packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
Outdated
Show resolved
Hide resolved
adamziel
reviewed
Mar 4, 2022
packages/block-library/src/navigation/edit/use-convert-classic-menu-to-block-menu.js
Outdated
Show resolved
Hide resolved
adamziel
reviewed
Mar 4, 2022
|
Thanks for catching (excuse the pun) those lost errors @adamziel. Let me rework those. |
8 tasks
adamziel
reviewed
Mar 4, 2022
| setError( err?.message ); | ||
| setStatus( CLASSIC_MENU_CONVERSION_ERROR ); | ||
|
|
||
| // Rethrow error for debugging. |
There was a problem hiding this comment.
nit, this could say just throw err since we're not going to tell the user what the error message says.
|
The only remaining thing I don't like is how many times it says classic menu conversion. I don't want to block on that, though, especially since I don't have any shorter naming scheme to propose. Edit actually, I left a few more notes |
adamziel
reviewed
Mar 4, 2022
adamziel
reviewed
Mar 4, 2022
adamziel
reviewed
Mar 4, 2022
I agree. I think perhaps a var naming follow up is in order. Especially when you see which way #39219 is heading. |
adamziel
reviewed
Mar 4, 2022
adamziel
reviewed
Mar 4, 2022
|
I'm going to merge this once tests pass unless I get any more ❌ feedback. |
|
I saw the ping last week and did not have time to look at it. From a quick glance it looks good:
The following issue is present in trunk, and subject to separate conversations, so not something that should block this PR. But showing the old menu even after it's being converted, as well as tools to manage them, it adds a fair bit of confusion: when do those tools appear? Why can I convert my menu again, wasn't it converted already? Worth us all looking at separately, but not precluded by these enhancements. Nice! |
No worries.
👍
Agreed. I feel like there's an Issue tracking this but I can't find it right now. |
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.
Description
Alternative approach to #38824. Continues working towards addressing #37190.
This PR seeks both to
The current hooks are complex to reason about and don't expose enough information about the progress of the conversion to allow for the UI to respond (e.g. loading states...etc).
This PR builds on the approach outlined by @talldan in #38824 (review). Essentially we create a new hook which exposes:
🙏 Please note: This PR does not attempt to solve the loading state indicator problems that are noticeable in this PR. This is being addressed in #38907.Outstanding problems to solve
Create loading UI states in the block whilst the conversion process happens (good things we expose the state!).To be addressed following merge of Improve Nav block loading and placeholder states #38907..catch().etc).I will tackle those steps during the remainder of this week.
Testing Instructions
Menusscreen. Add lots of items just for fun.trunkor watch the screencaptures below).Screenshots
Before
Notice how the dropdown doesn't close and UI seems to lock up/freeze when I click the classic menu.
Screen.Capture.on.2022-02-17.at.10-36-05.mp4
After
Notice the UI lock up is gone. The dropdown disappears and we see the block loading state immediately which resolves as appropriate:
Screen.Capture.on.2022-03-03.at.15-59-01.mp4
Error handling
Notice that the UI communicates the error if the request fails:
Screen.Capture.on.2022-03-03.at.16-29-15.mov
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).