Skip to content

Fix the navigation issue inside cover blocks #66093

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.

Sign up for GitHub

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

Merged
aaronrobertshaw merged 8 commits into from
Oct 17, 2024
Merged

Fix the navigation issue inside cover blocks #66093

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eb4dc79
Implement a simple solution for the navigation open inside the cover
renatho Oct 11, 2024
64c14d3
Add the has-modal-open to the editor reproducing the same behavior of…
renatho Oct 11, 2024
b6f62e2
Add test
renatho Oct 14, 2024
10e1007
Remove not necessary comment
renatho Oct 15, 2024
0a244ec
Update check to see if second cover inner blocks container is clickable
renatho Oct 15, 2024
0850938
Add useEffect cleanup to remove modal class
renatho Oct 16, 2024
efe0a9e
Refactor html element selector
renatho Oct 16, 2024
eaabc88
Make sure element exists before trying to remove the class
renatho Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Refactor html element selector
  • Loading branch information
@renatho
renatho committed Oct 16, 2024
commit efe0a9ea417413c76635ba1e0fda5c52ff551ec9
2 changes: 1 addition & 1 deletion packages/block-library/src/navigation/edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ function useResponsiveMenu( navRef ) {
return;
}

const htmlElement = navRef.current.closest( 'html' );
const htmlElement = navRef.current.ownerDocument.documentElement;

// Add a `has-modal-open` class to the <html> when the responsive
// menu is open. This reproduces the same behavior of the frontend.
Comment on lines +87 to +88

@aaronrobertshaw aaronrobertshaw Oct 15, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does feel a bit odd for a block to be manipulating the root node but if this brings it inline with the frontend perhaps it's already been decided this is ok?

@getdave getdave Oct 15, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does feel unusual. Is there any precedent?

@getdave getdave Oct 15, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually needed? I used the testing instructions against trunk and was able to replicate the issue but only on the front of the site and not in the Editor.

Screen.Capture.on.2024-10-15.at.13-07-37.mp4

@renatho renatho Oct 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it's really needed because also happens in the editor.

Screenshot 2024-10-15 at 09 51 06

Since it's the behavior in the frontend, I think it would be fine to have the same behavior replicated in the editor.

I used the testing instructions against trunk and was able to replicate the issue but only on the front of the site and not in the Editor.

I noticed sometimes some kind of cache. I didn't dig into to figure out, but sometimes when it delayed I restarted the build and restarted the browser, also marking the "Disable cache" in my network tab.

@getdave getdave Oct 15, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there specific steps to replicate that state? I tried following the PR instructions but it didn't manifest. Maybe it was prior to the rebase?

@renatho renatho Oct 18, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I raised this PR. I'd appreciate if someone could review it to improve the solution.

I also tried to update the fixtures through npm run fixtures:regenerate, but I received some errors. If someone could explain me what is happening and how I could fix, it would also be very helpful to save some time. =)

@aaronrobertshaw aaronrobertshaw Oct 21, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked I was wrong about the deprecation and it's transparent for the user

Yep, the only real sign within the editor should be a console message stating the block was migrated.

And the solution of the has-modal-open will be only for backward compatibility

This PR says it should be in the 19.6 release. So if we do go with the alternative solution soon I don't think there'll be any need to keep the .has-modal-open class styles around for BC. The change introducing it won't have been released in other words.

I'll comment to that effect when I get to review the alternate PR.

I also tried to update the fixtures through npm run fixtures:regenerate, but I received some errors.

Without looking I can't say if this is the reason or not but another misconception about deprecations is that they are chained together to migrate one deprecated version to the next, all the way to the latest version.

Instead, all deprecations need to be updated (if required) to migrate that deprecated version to the latest. We can continue the discussion over on #66249 as needed.

Thanks for spinning it up 🚀

@renatho renatho Oct 21, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR says it should be in the 19.6 release. So if we do go with the alternative solution soon I don't think there'll be any need to keep the .has-modal-open class styles around for BC. The change introducing it won't have been released in other words.

It's good to have that anyway, so it's fixed everywhere not needing a manual save in all the broken posts/templates. :)

@aaronrobertshaw aaronrobertshaw Oct 22, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't the original issue only in the editor? If that's the case, the deprecation will migrate and fix that display without needing further styles.

@renatho renatho Oct 22, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Connecting the points, I sent an answer here: #66249 (comment)

Expand Down