-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix mover position. #30242
Fix mover position. #30242
Conversation
Size Change: +60 B (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd noticed this as well, but forgotten to make an issue. Thanks for fixing 🎉
@@ -556,7 +556,8 @@ | |||
|
|||
// Position mover arrows for both toolbars. | |||
.block-editor-block-contextual-toolbar, | |||
.edit-post-header-toolbar__block-toolbar { | |||
.edit-post-header-toolbar__block-toolbar, | |||
.edit-site-header-toolbar__block-toolbar { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not something for this PR, I can follow up, but I just realised this is where we probably also need edit-widgets-header__block-toolbar
and edit-navigation-layout__block-toolbar
. Hopefully that would solve some of the issues that have been seen.
It doesn't seem completely sustainable to style the toolbar using these classnames. It ties in with an issue I made - #29965. But not something to worry about right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the review.
I profoundly agree it's not sustainable. I forgive the current situation because the dedicated site editor as a separate menu item was created in order to experiment. But as we move forward and if we decide to keep that separate menu item, we need to find a better way to do that. This separate PR I worked on is an example of that: #30234 — the markup deviates quite a bit, and it's painful to keep in sync; features or bugfixes for one editor built for one are likely to be missing from the other due to this mix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I just merged with the big green button — let me follow up with a PR with your suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followup: #30284
Description
The movers were not positioned right:
This PR fixes that, and an issue for the site editor:
How has this been tested?
Please test the mover control in post and site editor, mobile and desktop, top toolbar on and off, and verify it all looks good.
Checklist: