-
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
Add a landing section to stylebook tabs #66545
Conversation
3247cde
to
373c6b2
Compare
Size Change: +986 B (+0.05%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
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. |
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.
This is fantastic straight out of the box, thanks for working on it.
I feel it's in a pretty good place to merge. Keen to hear what other folks think
I left a few minor questions that are not blockers.
return ( | ||
example.category !== 'landing' && | ||
! landingCategoryExamples.examples.find( | ||
( landingExample ) => landingExample.name === example.name |
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 work filtering out duplicates on the single page 💯
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.
Should we memoize the collection of these deduped single page examples?
@@ -107,6 +107,11 @@ export const STYLE_BOOK_THEME_SUBCATEGORIES: Omit< | |||
]; | |||
|
|||
export const STYLE_BOOK_CATEGORIES: StyleBookCategory[] = [ | |||
{ | |||
slug: 'landing', | |||
title: __( 'Landing' ), |
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 know "Landing" has been bandied about, but what does it mean exactly to a user?
From #53431:
It serves as a quick glanceable "poster view" for the site style guide
I don't have a better alternative — 'Overview'? — but I suppose it could go in the direction of "Theme style guide" if and when we allow patterns and/or theme devs to add to 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.
Personally, I think I lean towards "Overview" rather than "Landing" as being more meaningful for more people.
IIRC there was some talk recently about quirky things and personality being something missed lately. A left field option could be "At a glance". It's not a single word but is still relatively short. Once it's translated that could be a different story though.
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.
Great question. I'll just ping @richtabor for discussion on the name for the category as he often has good ideas for these things!
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.
Connecting some dots to my wall of text here, I expect the tabs to eventually go away in favor of drilldown-based contextual navigation. Instead of clicking a tab, you click "Typography" in global styles, and the preview updates to match.
In that vein, the name of the tab stops becoming user visible.
title: __( 'Headings' ), | ||
category: 'landing', | ||
blocks: createBlock( 'core/heading', { | ||
content: `AaBbCcDdEeFfGgHhIiJjKkLlMmNnOoPpQqRrSsTtUuVvWwXxYyZz0123456789{(...)},?!*&:;_@#$`, |
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.
Wondering if this should be a translatable string? Mainly for the letter characters in case it's important to show off certain characters, e.g., umlauts, or use language-specific alphabets.
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.
+1 on the translation front. I think it definitely fits with the overall extensibility and customization goals for the Style Book.
title: __( 'Paragraph' ), | ||
category: 'landing', | ||
blocks: createBlock( 'core/paragraph', { | ||
content: `There was an Old Man of Vienna, |
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.
👍🏻
Should we make this translatable too?
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.
+1
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.
Noting some thoughts here, that are related.
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 work @tellthemachines ✨
This tests very nicely for me as well.
My vote would be to make the small tweaks @ramonjd noted (e.g. naming & translation), merge, and then iterate from there.
As an aside, it feels a bit odd to me having the color palettes split on the single page view of the Style Book. That's per the design at this point so not an issue for this PR, just something I hope comes out in the wash during future iterations.
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 work here, this is looking really good to me, too!
Not introduced in this PR I'm sure, but I noticed while testing that the break point for smaller sizes when reducing the resizable frame is a bit squished / doesn't fill out the horizontal space available:
Edit: looks like it was the change to the width rule. I've added a comment (#66545 (comment)) 🙂
Nice, good progress! It's unclear to me how much we need to accomplish in one PR vs another, knowing that at the end of the effort we want to be as close to the following as possible: The above is perhaps easier to see in Figma, here. Focusing first on this landing page, it might just be nicer to get closer to those initial mockups visually. Mainly standing out is that the landing page is heavily curated, showing both Heading and Paragraph blocks in a column pattern, rather than just as lists of blocks. Precisely what we show as text there is a separate task on its own. The old man of Senna is a fun little blurb, but to better match this pattern, we'll want something longer. And since we don't support We do want custom text on this landing page, and it should be translatable ideally. But we also want to get closer to the pattern. If not to start, then later, so at some point translations of this initial piece will become obsolete. There's also a new treatment for the style book headings, featuring horizontal separators. Note also how the new style book designs appear to be leveraging wide and content width alignments, with everything on the landing page being wide, but many blocks and pieces on subpages being content-width. Example: At some point, this landing page should be the default thing you see in the new style book location in Site View. As part of this effort we are replacing the tabs at the top, with contextual navigation of the new left sidebar. In this view, the behavior is like so:
Visually going through the flow, you start here: Click Typography, and you see the contents of the typography tab (note: rough mockup!): Click Colors, and you see the contents of the color tab (note: rough mockup!): Click Blocks → Navigation, and you see an exploded view of the navigation block (note: rough mockup!): The navigation block with states is related to this. Where we place media and site identity, is as-yet unclear. So we may indeed want to include those on the landing page, for now. Separate to this PR, but related as far as tasks, there's an effort needed to update and improve the default placeholder content for many of our blocks. Referring to mont blanc here: Whether we can use the images @beafialho included in her mockup, we need to check the license. But we definitely should update these, as they are far better: Some of these pieces would be good to get @beafialho advice on, in case there's any nuance that has changed since she created the initial mockups. But the main point is: the landing page is more like a block editor page with patterns, where all the other subsections show the full breadth of blocks available, even if they also have somewhat updated categories. In summary There are some immediate tasks to get closer to the landing page curation and layout as outlined, specifically this layout: The heading and subheading style is one task, applying a heading + columns pattern is another. @beafialho not sure how busy you are, but would you be able to work with @tellthemachines to supply such a pattern? If you can build it in the editor, I imagine we can use it here. As far as the other tasks—updating the drilldown behavior to retire the tabs, showing an exploded view of block states in the Blocks drilldown—we can follow up with separately, and probably the task of updating default content (especially for Image and Gallery blocks) I or some other designers can do. Does that breakdown make sense? Thanks for working on this! |
Thanks for the ping @joen, I double checked the image sources and just in case I have replaced the ones that had the "Free" label on RawPixel with similar ones that are Public Domain. That way, we can use them all. Here are the links if they happen to be necessary:
Just to clarify, do you mean building this pattern in the editor? |
Yes exactly, I think having this pattern would be useful. It's unclear whether we can/should use |
For the stylebook example I'll be creating the blocks dynamically with |
5033096
to
9dada60
Compare
Looks good. Would it be good to extract these notes into one or several issues, so anyone can pick them up? Thank you for updating the text, looks great. I guess we don't need your pattern after all, @beafialho! One thing we do need to figure out, and you mention this yourself: we need to thread the needle on exactly what text to show here. How about this text, at least for now, and then other PRs can update them if they have further improvements.
That would look about like this: It's not quite perfect, since the left paragraph is longer than the right one. You mentioned potentially using |
The text you suggested @jasmussen also looks good to me, but just to offer some context, my initial idea was to use some informative/educational content about each block as demo text. I did that for the headings, post content, quote and pullquote blocks: For headings, the full text would be:
|
Ah, makes perfect sense. Though we still need the precise text you authored for those two paragraphs, though. And either it's one paragraph split using |
Sure, I used the text below. If I'm understanding correctly, it's a problem that it starts mid-sentence? In the design, that would be fine because as you increase or decrease text size, the paragraphs would normally redistribute across columns, like this example.
font-sizing.mp4 |
The problem is in the PR currently, it's these two "paragraphs":
An ideal solution would be this, i.e. use Another solution would be to break the text at a proper period. Like so:
How do you feel about the latter? |
I see! Yes, that sounds good to me. |
This isn't an existing feature in the block editor though, is it? Wouldn't it be better to stick to a presentation that is reproducible with blocks in the UI? Otherwise folks might think that they can have 2-col Paragraphs, when that isn't possible without some hacky extra CSS. |
FWIW a while back text column (column-count) support was added to typography block supports. Efforts there weren't a high priority so it was put on the backburner and forgotten about. There are still open issues to support it for blocks like paragraph and post-excerpt. If I'm not forgetting any blockers, I don't think it would take a lot to enable that support. The question is if we really want that even as an optional (hidden by default) typography control on the paragraph block. |
I had no idea this existed 😮 I guess we could consider adding support to the Paragraph block (was there any reason not to do so initially?), but I'm not sure how useful it'll be without a wide alignment control because with a standard content width two columns look a little squished: For now, I'll go with updating the text to the two separate paragraphs mentioned above and we'll see how that works. |
I've pushed an update that changes the name of the "Text" section to "Typography" to better match the corresponding global styles section, and linked it to typography in global styles. This should now be ready for a final round of testing and reviews! |
Flaky tests detected in f6ce5b1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11773167039
|
} else if ( blockName === 'typography' ) { | ||
// Go to typography Global Styles. | ||
onPathChange( '/typography' ); | ||
return; | ||
} |
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.
} else if ( blockName === 'typography' ) { | |
// Go to typography Global Styles. | |
onPathChange( '/typography' ); | |
return; | |
} | |
} | |
if ( blockName === 'typography' ) { | |
// Go to typography Global Styles. | |
onPathChange( '/typography' ); | |
return; | |
} |
I think the else if
is redundant due to the previous early return?
Also it looks like the /typography
route handling needs to happen in the editor too:
gutenberg/packages/edit-site/src/components/global-styles/ui.js
Lines 194 to 195 in 2a18aae
} | |
onSelect={ ( blockName ) => { |
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.
Fixed, thanks for spotting 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.
🚢
This is working well for me in both view and edit modes of the site editor. Thank you!!
Tested in Firefox, Chrome, Safari and Edge.
There's just one outstanding update to add the /typography
route to editor style book onSelect
callback.
Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: beafialho <beafialho@git.wordpress.org>
What?
Closes #66517.
Testing Instructions
Given that the stylebook view in the left hand sidebar has everything in one long scroll, I deduped the items from the Landing section and added them in at the top.
Screenshots or screencast