Skip to content
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

Fix text alignment in the Site Editor sidebar #48959

Merged
merged 2 commits into from
Mar 9, 2023
Merged

Fix text alignment in the Site Editor sidebar #48959

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4204ded
Fix text alignment
jameskoster Mar 9, 2023
52911ef
Remove width width override
jameskoster Mar 9, 2023
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
1 change: 1 addition & 0 deletions packages/edit-site/src/components/sidebar-navigation-item/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
.edit-site-sidebar-navigation-item.components-item {
color: $gray-600;
border-width: $border-width-tab;
margin: 0 2.5px; // This combines with the border above to create a 4px gap and ensure the text line up nicely with other side bar elements along the Y axis
Copy link
Contributor

Choose a reason for hiding this comment

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

Ack, this one's kinda ugly. It looks like this is to enable the focus style. I think that actually puts our metrics off in other places too, like the 40px item-height. If it's a nightmare to refactor the focus style to use a box-shadow + transparent outline (for high contrast mode) like our buttons do, I'm happy to approve this one. But that seems like a refactor to do eventually, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Item calls a Button. So I don't really understand why this transparent border is being added here, I'd expect the focus style to be inehrent to the Button component.

I'll see if I can move it to a pseudo element, but that feels a bit like replacing one hack with another xD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems the border wasn't actually required, so I removed it. A small margin is still required for the alignment though.

border.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice, you solved it, let me review in a second. This one:

I'll see if I can move it to a pseudo element, but that feels a bit like replacing one hack with another xD

It should be possible to use box-shadow for the focus style, which wouldn't affect the footprint whereas border does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just removed the border.

The focus style now comes from .css-1xahv0o-View-unstyledButton-medium-item-spacedAround:focus which seems to be connected to the experimental ItemGroup / Item components.

I agree it would be nice to update that to use box-shadow rather than the current border implementation. But since that's at the component level it should probably be handled in a dedicated PR?


&:hover,
&:focus,
Expand Down
6 changes: 5 additions & 1 deletion packages/edit-site/src/components/site-hub/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,11 @@ const SiteHub = forwardRef( ( props, ref ) => {
</Button>
</motion.div>

{ showLabels && <div>{ siteTitle }</div> }
{ showLabels && (
<div className="edit-site-site-hub__site-title">
{ siteTitle }
</div>
) }
</HStack>
</motion.div>
);
Expand Down
4 changes: 4 additions & 0 deletions packages/edit-site/src/components/site-hub/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,7 @@
white-space: nowrap;
overflow: hidden;
}

.edit-site-site-hub__site-title {
margin-left: $grid-unit-05;
}