-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Update site hub action positioning #60511
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
@@ -3,7 +3,6 @@ | |||
align-items: center; | |||
justify-content: space-between; | |||
gap: $grid-unit-10; | |||
overflow: hidden; |
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.
@youknowriad you added this recently but I noticed it was cutting off the focus ring on the search button. Removing it doesn't seem to have caused any issues. Do you remember why it was added?
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 think this was important for long site titles, did you try that
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.
Maybe it's fine to keep it but the icons are pushed to the right, so you'll need a flex-shrink: 0
somewhere.
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 think there's a flex-grow rule that is now unnecessary in .edit-site-site-hub__site-view-link
as well
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.
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.
yes by adding flex-shrink: 0 to your new hstack it will solve the long titles issue.
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.
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.
Still not seeing a need for this overflow style 🤔
@@ -156,25 +156,28 @@ const SiteHub = memo( ( { isTransparent, className } ) => { | |||
<div className="edit-site-site-hub__title"> | |||
{ decodeEntities( siteTitle ) } | |||
</div> | |||
<HStack spacing={ 0 } expanded={ false }> |
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.
Is the nested HStack necessary?
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 don't mind it, just trying to understand why we need 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.
It's necessary to retain a small gap between the site title and the action buttons which is probably important when the title is very long. Could add a small margin to .edit-site-site-hub__title
instead if that's preferable.
Size Change: +23 B (0%) Total Size: 1.73 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.
Nice one.
<HStack | ||
spacing={ 0 } | ||
expanded={ false } | ||
style={ { flexShrink: '0' } } |
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.
Worth adding a class for this, or okay inline?
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 don't know, I'd say just add a class but I don't care much either.
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 suppose a class might be handy in the future...
Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org>
The spacing of icons in the sidebar is a bit messy, this PR aims to start tidying that up by updating the site hub.
Before
Notice how the search icon is not aligned with the plus / chevron from other panels.
After
Notice the search icon is now aligned with the plus / chevron. I also reduced the gap between search / view buttons.