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.

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 Gutenberg Stories #590

Merged
merged 39 commits into from
Apr 9, 2020
Merged

Add Gutenberg Stories #590

merged 39 commits into from
Apr 9, 2020

Conversation

Josh68
Copy link
Contributor

@Josh68 Josh68 commented Apr 3, 2020

Overview

This PR adds a single stories file containing un-styled stories for readily theme-able Gutenberg blocks. These are blocks that generate markup with added wp-block- classes.

The examples aim to provide ample detail for understanding variations that may be generated using the block editor controls. However, it is not exhaustive, and the API for each block is not easily discoverable. In almost all cases, the markup and classes added to examples is based on discovery using the editor on a local Wordpress installation.

Screenshots

image

Testing

  1. View the preview theme
  2. See cloudfour.com-wp issue #450
  3. Ensure all non-widget core blocks with a 🎨 icon have a story
  4. Provide feedback regarding copy, examples (easily updated), and any other suggestions

/CC @tylersticka @derekshirk

@Josh68 Josh68 self-assigned this Apr 3, 2020
@Josh68 Josh68 requested a review from tylersticka April 3, 2020 23:42
@Josh68 Josh68 changed the title [DRAFT] Add Gutenberg Stories Add Gutenberg Stories Apr 3, 2020
@Josh68 Josh68 marked this pull request as ready for review April 4, 2020 00:11
@Josh68 Josh68 requested a review from derekshirk April 4, 2020 00:11
@Josh68
Copy link
Contributor Author

Josh68 commented Apr 4, 2020

Added @derekshirk as a potential reviewer, but I think @tylersticka needs to be the primary. If I should add any one else (or remove Derek), let me know.

Copy link
Member

@Paul-Hebert Paul-Hebert left a comment

Choose a reason for hiding this comment

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

This is looking good Josh!

I don't have a ton of context on this work but I was curious to see how this was set up. Because of that I'm not going to approve or request changes but I left some nits and questions inline.

I wonder if it would be possible to add a class knob so we could explore adding pattern library classes to these blocks? e.g. making the button block have the c-button class? Though that may well be outside the scope of this PR.

Anyways, nice job putting this together! It was interesting to see the various blocks Gutenberg provides.

## Button

The button block creates one or more `div`s with class `wp-block-button`, each containing
an `a` element with a class of `wp-block-button__link`.
Copy link
Member

Choose a reason for hiding this comment

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

It's weird that this doesn't use <button>. 🤔

That's what Gutenberg gives us by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You got it. In this case, button is meant to be a link, either to directly get a media asset or navigate to its attachment page. It could probably use a better name.

Copy link
Member

Choose a reason for hiding this comment

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

The intent here is to add call-to-action style links to a post. The containers exist for layout, the links to link to that content. I think in this case the naming of this pattern is probably more confusing to web developers than to end users.

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 can update the description, if that seems worthwhile.

>
</blockquote>
<iframe
title=""Elements” — wp-themes.com"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: looks like these opening quotation marks got messed up somehow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. This markup was actual output from embedding wp-themes content, so not sure why it came out that way.

Side note: I imagine we may have discussions about what sources we keep or change in this PR, but my goal was to try to offer relatively complete examples with content, and wordpress.org, unfortunately, does not itself offer much good content to use in stories like this.

class="wp-image-130"
srcset="
https://s.w.org/images/core/5.3/MtBlanc1.jpg 500w,
https://s.w.org/images/core/5.3/MtBlanc1-300x225.jpg 300w
Copy link
Member

Choose a reason for hiding this comment

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

It's cool that this is set up to use srcset!

(__)\\ )\\\/\\
||----w |
|| ||
</pre>`}
Copy link
Member

Choose a reason for hiding this comment

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

😆

`wp-block-pullquote`.

[Color controls](#color-settings) allow the setting of pre-defined or custom `background-color` and
text `color`. A styles control adds one of two wrapper classes, `is-style-default` or
Copy link
Member

Choose a reason for hiding this comment

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

Nit: sometimes this is referred to as styles control and sometimes it's referred to as style controls. It would be nice to be consistent.

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 updated for consistency

class="wp-block-spacer"
></div>
<p>Content after spacer.</p>`}
</Story>
Copy link
Member

Choose a reason for hiding this comment

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

Weird... is this meant to control margins in our WYSIWYG editor? Hopefully we can avoid this and use our margin utilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is simply a way to add a random amount of vertical space, but potentially with additional styling. And it's probably something that won't get used, but I don't know. In general, I don't know whether we might un-register some of the default blocks in our theme to prevent their use. TBD.

Copy link
Contributor Author

@Josh68 Josh68 left a comment

Choose a reason for hiding this comment

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

This is looking good Josh!

I don't have a ton of context on this work but I was curious to see how this was set up. Because of that I'm not going to approve or request changes but I left some nits and questions inline.

Thanks for the feedback, @Paul-Hebert. I'm curious to see how the pattern library and Gutenberg end up coming together.

I wonder if it would be possible to add a class knob so we could explore adding pattern library classes to these blocks? e.g. making the button block have the c-button class? Though that may well be outside the scope of this PR.

I'd like to see what @tylersticka thinks about this idea. All blocks do have a place to enter arbitrary class names, so this may make sense.


# WordPress Gutenberg Blocks

The Wordpress Gutenberg editor is a block-based page builder. The editor comes with
Copy link
Member

Choose a reason for hiding this comment

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

Can we link to some documentation somewhere in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a link to the main page on wordpress.org, but if you'd like to link to something more specific to development, let me know.

<Preview>
<Story name="core/audio" height="150px">{`
<figure class="wp-block-audio type">
<audio controls="" src=""></audio>
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a short demo audio file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

## Button

The button block creates one or more `div`s with class `wp-block-button`, each containing
an `a` element with a class of `wp-block-button__link`.
Copy link
Member

Choose a reason for hiding this comment

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

The intent here is to add call-to-action style links to a post. The containers exist for layout, the links to link to that content. I think in this case the naming of this pattern is probably more confusing to web developers than to end users.

Comment on lines 85 to 93
{`<pre class="wp-block-code">
<code>
// A "block" is the abstract term used
// to describe units of markup that,
// when composed together, form the
// content or layout of a page.
registerBlockType( name, settings );
</code>
</pre>`}
Copy link
Member

Choose a reason for hiding this comment

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

Whitespace should be stripped, as currently the pattern is showing a bunch of leading spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to remove whitespace. That was a formatting error on my end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#590 (comment)

Updated again. I eschewed using Prism, but can add if you like.

Comment on lines +80 to +81
The code block adds the class `wp-block-code` to a `pre` tag wrapping content inside
`code` tags.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's necessary to describe in prose the markup structure and attribute contents for each pattern?

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 wonder if we should discuss my overall approach to story descriptions here. I didn't think mine was necessarily the best, but felt like some basic introduction was a good idea for all of them, and that for the simplest patterns, this approach was just a way to be consistent across descriptions. I realize that for something like the code block, probably no description is needed at all.

In this case, would you even include a description, and if so, would it just be something like Provides a block-level class name for styling code snippets?

Comment on lines 87 to 91
// A "block" is the abstract term used
// to describe units of markup that,
// when composed together, form the
// content or layout of a page.
registerBlockType( name, settings );
Copy link
Member

Choose a reason for hiding this comment

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

I recommend choosing one of the code samples from our code block story instead of on that is 90% comment. (You can view different examples via a knob.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<Preview>
<Story name="core/file" height="100px">
{`<div class="wp-block-file">
<a href="https://i1.wp.com/wordpress.org/news/files/2020/03/Squares.jpg"
Copy link
Member

Choose a reason for hiding this comment

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

Let's avoid hotlinking to images in favor of storing some examples in our project. There should be plenty if you browse around our articles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, I avoided uploading assets into the repo (wasn't sure that was OK), but thought about the drawbacks of hotlinking. I will look to add images, audio, and video, as needed, because I think they're needed.

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've localized all previously hotlinked assets under static/media

@Josh68
Copy link
Contributor Author

Josh68 commented Apr 8, 2020

@tylersticka, please look at the updated audio and button descriptions and let me know if these provide the level of detail you'd like (avoiding too much prose description of the default generated markup). I will address others once you provide feedback on those.

I was assuming documenting some of the options that controls offer still made sense, but if you still think that's overkill, I can pare down further.

I think I've addressed all other comments so far. Feel free to resolve comment threads you've reviewed, if it helps. Thanks

@Josh68 Josh68 requested a review from tylersticka April 8, 2020 20:56
@Josh68 Josh68 merged commit c955c60 into v-next Apr 9, 2020
@Josh68 Josh68 deleted the add-gutenberg-stories branch April 9, 2020 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants