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

Add button interactive states in the stylebook #67541

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
+99 −2
Open

Add button interactive states in the stylebook #67541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7869490
Add button states in the stylebook design section
matiasbenedetto Dec 3, 2024
b1c5dbd
improving button state titles
matiasbenedetto Dec 4, 2024
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
81 changes: 81 additions & 0 deletions packages/edit-site/src/components/style-book/button-examples.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/**
* WordPress dependencies
*/
import { createBlock } from '@wordpress/blocks';
import { __experimentalGrid as Grid } from '@wordpress/components';
import { View } from '@wordpress/primitives';
import {
BlockList,
privateApis as blockEditorPrivateApis,
} from '@wordpress/block-editor';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { unlock } from '../../lock-unlock';

const { ExperimentalBlockEditorProvider, useGlobalStyle } = unlock(
blockEditorPrivateApis
);

const BUTTON_STATES = [
{
key: 'default',
title: __( 'Button' ),
Copy link
Member

Choose a reason for hiding this comment

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

Since there's block title of "Button" do you think these labels could just describe the state? E.g., 'Default', 'Active', 'Hover' etc

Screenshot 2024-12-06 at 1 38 00 pm

},
{
key: ':active',
title: __( 'Button (Active)' ),
},
{
key: ':any-link',
title: __( 'Button (Any Link)' ),
},
{
key: ':focus',
title: __( 'Button (Focus)' ),
},
{
key: ':hover',
title: __( 'Button (Hover)' ),
},
{
key: ':link',
title: __( 'Button (Link)' ),
},
{
key: ':visited',
title: __( 'Button (Visited)' ),
},
];

function ButtonExamples() {
const [ elementsButton ] = useGlobalStyle( 'elements.button' );
const blocks = BUTTON_STATES.map( ( { key } ) => {
const styles =
( key !== 'default' ? elementsButton[ key ] : elementsButton ) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when we have both a "default" and a "state", are we merging the state styles with the "default" styles (I just want to confirm that this is covered because it's not clear by reading this code)

{};
return createBlock( 'core/button', {
Copy link
Member

Choose a reason for hiding this comment

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

Something to keep in mind: the block might be deregistered and could trigger errors.

See:

text: __( 'Call to Action' ),
Copy link
Member

Choose a reason for hiding this comment

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

Better than the above still, instead of call to action, make the button text the state. The downside is that the buttons' widths will vary, but the CSS could be tweaked a bit e.g.,

Screenshot 2024-12-06 at 1 48 09 pm

Anyway, just an idea.

style: styles,
} );
} );

return (
<Grid columns={ 2 } gap={ 6 }>
{ blocks.map( ( block, index ) => (
<View key={ `button-example-${ BUTTON_STATES[ index ].key }` }>
<span className="edit-site-style-book__example-subtitle">
{ BUTTON_STATES[ index ].title }
</span>
<ExperimentalBlockEditorProvider value={ [ block ] }>
<BlockList appender={ false } />
</ExperimentalBlockEditorProvider>
</View>
) ) }
</Grid>
);
}

export default ButtonExamples;
5 changes: 5 additions & 0 deletions packages/edit-site/src/components/style-book/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,11 @@ export const STYLE_BOOK_IFRAME_STYLES = `
text-transform: uppercase;
}

.edit-site-style-book__example-subtitle {
Copy link
Member

Choose a reason for hiding this comment

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

If we make the button text describe its state we wouldn't need this CSS anyway.

font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, Oxygen-Sans, Ubuntu, Cantarell, "Helvetica Neue", sans-serif;
font-size: 9px;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It is very weird that we're rendering the titles and subtitles... within an iframe. Do we have any context on this decision? I think it would have been a lot simpler to avoid an iframe for style book and only use iframes for the examples (maybe even use BlockPreview rather than BlockList and Provider)

For clarity, I'm not asking of any change in this particular PR but I wonder if this was considered and if we didn't, we should probably consider as part of our efforts to improve style book. cc @tellthemachines @ramonjd @aaronrobertshaw

Copy link
Contributor

Choose a reason for hiding this comment

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

Speaking of the font size, it's a bit small. The later mockups have two heading sizes: the main category (16px), and the subheading size (13px):
Screenshot 2024-12-05 at 10 58 41

Potentially this can be separate since there's a lot of separate work going on with headings, but there's a spec here.

Copy link
Member

Choose a reason for hiding this comment

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

Do we have any context on this decision? I think it would have been a lot simpler to avoid an iframe for style book and only use iframes for the examples

It's an abstraction of noisysock's original implementation.

🤔 If I recall correctly, at the time it was to simulate an "editor instance" within the editor, but isolated so as to have more control over window resizing, styles and so on. It proved useful for revisions where discrete global styles configs could be loaded into the iframe without affecting the main canvas.

As is with many features, things just grew from there.

I think it would have been a lot simpler to avoid an iframe for style book and only use iframes for the examples (maybe even use BlockPreview rather than BlockList and Provider)
I wonder if this was considered and if we didn't, we should probably consider as part of our efforts to improve style book

I agree the setup is ripe for refactoring.

I tried a few variations while researching for #62216. I think it's worth reigniting, so maybe that issue could be extended/repurposed.

It will be a whole lot simpler if Global styles (including revisions and style book) finds a new home in the left-hand "view" site editor side bar.

There have been noises to that effect, but I doubt it will happen soon.

By "simpler" I mean that the component wouldn't have to live in the editor canvas and do all the iframe/slot faffery.

Copy link
Contributor

Choose a reason for hiding this comment

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

By "simpler" I mean that the component wouldn't have to live in the editor canvas and do all the iframe/slot faffery.

I feel like refactoring the stylebook to use multiple small "Block Previews" rather than a huge iFrame for everything is independent from whether it should be using the EditorContentSlotFill or not.

Copy link
Contributor

@tellthemachines tellthemachines Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

From memory the main reason we went with a single iframe for everything was in order to correctly show the theme background, whether it's a plain color or a background image. It's particularly relevant for background images as it means the image won't be split up and contained inside multiple small frames, but will replicate what it looks like on the actual site:

Screenshot 2024-12-06 at 10 14 16 am

We'd have the same problem with gradient backgrounds too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah that's a good reason :) I wonder if we can have a background iframe positioned absolutely behind everything. (Anyway, I guess we can probably move this discussion to a separate thread/issue :p )


.edit-site-style-book__subcategory-title {
font-size: 16px;
margin-bottom: 40px;
Expand Down
15 changes: 13 additions & 2 deletions packages/edit-site/src/components/style-book/examples.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type { BlockExample, ColorOrigin, MultiOriginPalettes } from './types';
import ColorExamples from './color-examples';
import DuotoneExamples from './duotone-examples';
import { STYLE_BOOK_COLOR_GROUPS } from './constants';
import ButtonExamples from './button-examples';

/**
* Returns examples color examples for each origin
Expand Down Expand Up @@ -177,11 +178,14 @@ function getOverviewBlockExamples(
* @return {BlockExample[]} An array of block examples.
*/
export function getExamples( colors: MultiOriginPalettes ): BlockExample[] {
// Exclude default examples from block to include custom examples for those blocks.
const excludedDefaultExamples = [ 'core/heading', 'core/button' ];

const nonHeadingBlockExamples = getBlockTypes()
.filter( ( blockType ) => {
const { name, example, supports } = blockType;
return (
name !== 'core/heading' &&
! excludedDefaultExamples.includes( name ) &&
!! example &&
supports?.inserter !== false
);
Expand Down Expand Up @@ -227,10 +231,17 @@ export function getExamples( colors: MultiOriginPalettes ): BlockExample[] {
} ),
};
const colorExamples = getColorExamples( colors );

const overviewBlockExamples = getOverviewBlockExamples( colors );

const buttonExample = {
name: 'core/button',
title: __( 'Button' ),
category: 'design',
content: <ButtonExamples />,
};

return [
buttonExample,
headingsExample,
...colorExamples,
...nonHeadingBlockExamples,
Expand Down
Loading