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

Update the template title in the details panel #49487

Merged
merged 4 commits into from
Apr 3, 2023
Merged

Update the template title in the details panel #49487

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c3bee91
Update the template title in the details panel
jameskoster Mar 30, 2023
46c784f
Do not truncate
jameskoster Mar 31, 2023
cc561fa
Revert title size
jameskoster Mar 31, 2023
b49e78f
'grid unit'
jameskoster Apr 3, 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
12 changes: 9 additions & 3 deletions packages/edit-site/src/components/sidebar-navigation-screen/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
__experimentalHStack as HStack,
__experimentalVStack as VStack,
__experimentalNavigatorToParentButton as NavigatorToParentButton,
__experimentalHeading as Heading,
} from '@wordpress/components';
import { isRTL, __ } from '@wordpress/i18n';
import { chevronRight, chevronLeft } from '@wordpress/icons';
Expand Down Expand Up @@ -35,7 +36,7 @@ export default function SidebarNavigationScreen( {
<VStack spacing={ 2 }>
<HStack
spacing={ 4 }
justify="flex-start"
alignment="flex-start"
className="edit-site-sidebar-navigation-screen__title-icon"
>
{ ! isRoot ? (
Expand All @@ -52,9 +53,14 @@ export default function SidebarNavigationScreen( {
label={ __( 'Dashboard' ) }
/>
) }
<h2 className="edit-site-sidebar-navigation-screen__title">
<Heading
className="edit-site-sidebar-navigation-screen__title"
color={ 'white' }
level={ 2 }
size={ 16 }
>
{ title }
</h2>
</Heading>
{ actions }
</HStack>

Expand Down
6 changes: 1 addition & 5 deletions packages/edit-site/src/components/sidebar-navigation-screen/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,6 @@
}

.edit-site-sidebar-navigation-screen__title {
font-size: calc(1.56 * 13px);
Copy link
Contributor

Choose a reason for hiding this comment

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

This was a bit of a weird font size, a roundabout way to arrive at 20px, however I do miss those 20px font size. I thought that was more substantial as a detail page heading.

Copy link
Contributor Author

@jameskoster jameskoster Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Heh, I know it's subjective but the 20px feels kind of oversized to me, particularly with the longer titles.

I like 16 because it aligns nicely with the icons:

Screenshot 2023-03-31 at 15 17 59

It's also what we've been circling around with mockups like:

Screenshot 2023-03-31 at 15 20 18

Screenshot 2023-03-31 at 15 20 53

This is only a light pushback though, I don't mind working on that separately :)

Edit: It also means using non-grid-unit value to get the alignment right (6px rather than 8) 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

6px is a grid unit! It's $grid-unit-15 * 0.5 ;)

It's not a change I'd fight tooth and nail against. But mainly I think it's important we have some contrast between the various headings, just so we don't end up with a bunch of font sizes that are close but not quite contrasty enough. At the moment we have 11px labels (allcaps), 12px help text, 13px body size for the small stuff. Then we have a few sizes above here that don't seem entirely systematized, something around 14px for modal subheadings, and 19ish for modal headers. It's a bit messy and could use a system in our library. 16 may be a part of that, but then I'd roll it out more broadly. Just visually it doesn't seem far enough from 13px.

If you want the 16px change in this PR, I'm happy to make @richtabor or @SaxonF a tie-breaker :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

6px is a grid unit! It's $grid-unit-15 * 0.5 ;)

😆

Oh absolutely, we should define a scale, ideally built into the Text / Heading components. We have this in the Design Library, but like you said only the only ones close to being formalised in any way are Label, Helper text, and Body. We should revisit that now we're beginning to explore management screens in some more detail.

No need for a tie-breaker haha, I reverted.

Copy link
Member

@richtabor richtabor Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't mind the current state's 20px size. I share the same sentiment as @jasmussen — 16px may be too small.

line-height: normal;
font-weight: 500;
flex-grow: 1;
color: $white;
margin: 0;
padding: $grid-unit-10 0 0 0;
}