-
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
Updated styling to match the post editor #23525
Conversation
Size Change: +65 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
align-items: center; | ||
|
||
& > * { | ||
.edit-site-header-toolbar__inserter-toggle { |
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.
Why doesn't edit-post-header__toolbar
have similar styles?
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.
It does, but it's too specific and expects to be in a post-editor
container. :/
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 only see it when it has .has-icon
.
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.
@epiqueras What do you mean? I don't fully understand what you're asking or proposing here.
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.
edit-post-header__toolbar
only has these styles with .has-icon
. Shouldn't it be the same here?
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.
It looks like the .has-icon
selector is unnecessary. This element always has an icon. I can't think of a good reason to add it other than maybe it was an attempt to increase specificity to protect against theme style conflicts. Feel free to add it.
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.
Can we keep it consistent between both places? I.e., remove it from the post editor.
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.
Sure. @epiqueras that should probably be a separate PR, correct?
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.
Sure
The topbar and the sidebar accordions all looked great on my end. Thanks for the fix! |
This update feels much better 👍 |
@epiqueras Is there something I need to change to get this approved for merge? |
@MichaelArestad This comment needs clearing up. #23525 (comment) |
Fixes #23183 and a padding/spacing issue with the top bar inserter (and other buttons). Everything should be pretty close, now (though I do think they should share styles rather than repeat them).
Before
After