-
Notifications
You must be signed in to change notification settings - Fork 2k
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
DevDocs: add a Gutenberg blocks section #26065
Conversation
* Internal dependencies | ||
*/ | ||
import DocumentHead from 'components/data/document-head'; | ||
import Gutenberg from '../gutenberg'; |
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.
we'll probably want to differentiate this name here from the editor itself since Gutenberg
is the name of the whole project. we could make ../gutenberg-blocks
for example which I think would help clear up the confusion
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.
yep, it makes sense. I have just renamed everything to gutenberg-blocks
client/devdocs/gutenberg/README.md
Outdated
Gutenberg Blocks | ||
==== | ||
|
||
Lorem ipsum dolor sit amet, consectetur adipiscing elit. Praesent ut arcu et mauris fermentum tempus. In vel lacinia lorem, nec accumsan tortor. Nulla non tellus interdum, vestibulum nisl sed, placerat metus. |
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.
obviously this needs to be filled out before merging 😉
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.
my plan was to do that in a separate PR as I thought it was ok to have "lorem ipsum" content if that is behind a feature flag.
anyway, I have just added a proper documentation in order to get rid of the lorem ipsum ;)
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've just realized a problem we have with this: all the README.md files can be accessed even if the feature flag is not active.
I'll check later how can that be improved, so the documentation is not visible if the feature flag is not enabled
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.
it's not about feature-flagging it. it's about committing code that really has no purpose. it's fine to not include it, it's fine to take the time to write it, but it's not really a Good Thing™️ to ship code that's dead at the get-go.
my philosophy is that code that merges is permanent. there is no such thing as temporary code or "I'll get to it soon."
|
||
expect( readmeViewer ).toHaveLength( 1 ); | ||
expect( readmeViewer.props().readmeFilePath ).toBe( '/client/devdocs/gutenberg/README.md' ); | ||
} ); |
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'm not convinced on the value of these tests. I suspect that we'd know if the sections are missing because someone would intentionally be deleting them. further, what behaviors are these tests verifying? if it were me I'd probably leave them out.
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.
you're right, this is not checking any behavior.
I just have the habit of writing unit tests for everything but, as pointed here, that's not the goal if we want to strike the right balance between covering expected and unexpected behaviors, speedy execution and code maintenance.
config/development.json
Outdated
"devdocs/components-usage-stats": false, | ||
"devdocs/gutenberg": true, | ||
"devdocs/redirect-loggedout-homepage": true, |
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.
it's probably worth reordering the existing items in a separate PR so we don't mix the concerns in this one
sounds good to me. definitely better than leaving off the Gutenberg part
also seems like a good choice to me. we can easily change later if we want to. thanks for this work! it's cool to see it coming into Calypso! you may already know this but you can jump to the devdocs with the global hotkey sequence g d |
|
||
return ( | ||
<Main className={ className }> | ||
<DocumentHead title="Gutenberg Blocks"/> |
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.
Re: naming I think @mtias suggested "Gutenberg Components" for now, though I'm not sure how strongly he feels about 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.
Gutenberg have both blocks and components. It's my understanding that we want to show only the blocks according to the project introduction @ehg gave to me, as nothing was said about the components.
Having that in consideration, do we want to name the blocks from Gutenberg as "Gutenberg components" in Calypso? Wouldn't it be confusing?
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.
Blocks in Calypso (more than pure UI components / handle their own data needs) need to be renamed to something else, but I'll leave it up to the framework group to decide what else.
Wouldn't it be confusing?
Until this is resolved, yes, every variation is a bit confusing. 😄I don't have strong feeling here, but wanted to note earlier suggestions.
@@ -0,0 +1,26 @@ | |||
/** |
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.
If we add /** @format */
above this line, prettier, will handle whitespace formatting for us.
client/gutenberg-blocks/README.md
Outdated
The Gutenberg editor uses | ||
[blocks](https://github.com/WordPress/gutenberg/blob/master/blocks/README.md) to create all types | ||
of content. A Gutenberg block is the abstract term used to describe units of markup that, | ||
composed together, form the content or layout of a webpage. |
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 one is a little hard to condense in a single sentence, but maybe we can link out to additional sources like https://wordpress.org/gutenberg/handbook/language/ in future updates
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 adds a new feature flag
devdocs/gutenberg
and creates a new section in/devdocs
(if the feature flag is enabled) with dummy content.The new section will be used in the future for displaying how the Gutenberg blocks can be used in Calypso
Todo
devdocs/gutenberg-blocks
feature flag/devdocs/gutenberg-blocks
Add testsThey are not needed as there is no behavior to check yetTesting instructions
With the
devdocs/gutenberg-blocks
feature flag enabled:npm start
/devdocs
devdocs/gutenberg-blocks
and a page with documentation appearsWith the
devdocs/gutenberg
feature flag disabled:DISABLE_FEATURES=devdocs/gutenberg-blocks npm start
/devdocs
/devdocs/gutenberg-blocks
Questions
gridicon-grid
for the momentNext steps
Prototype what a single Gutenberg block should look like in this section on an experimental branch, copying over files if we really need to.