-
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: New "Gutenberg components" section #26314
Conversation
Can you try rebasing from master again? Looks like a number of misc commits on the branch. |
yeah, sorry @gwwar. not sure what happened. I'm fixing it |
No worries, can happen sometimes with an odd merge, but best to clean it up before it gets too gnarly. A Unless we're working together with another person on a branch, feel free to rebase freely as needed. |
ddd1d05
to
20a8b94
Compare
everything should be right now @gwwar. thanks for the help! |
@@ -104,6 +104,13 @@ export default class DevdocsSidebar extends React.PureComponent { | |||
link="/devdocs/blocks" | |||
selected={ this.isItemSelected( '/devdocs/blocks', false ) } | |||
/> | |||
<SidebarItem |
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 put this under the same feature flag?
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 had the same doubt. My main concern is that there are some things we cannot hide even if the feature flag is not enabled:
- Gutenberg components in the Components Glossary
- Gutenberg components docs in the search:
Because of that and because all the Gutenberg components are actually ready to be used in Calypso (we just need to add more docs explaining how to use the rest of them), I thought that was ok to display this publicly.
But I don't have a strong opinion about this, honestly. If you think that having the readme files available is not a big deal, I can put this under the same feature flag.
showEditLink = false; | ||
} else { | ||
readmeFilePath = `/client/${ section }/${ example.props.readmeFilePath }/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.
Maybe pull this out into a helper functions?
const shouldShowEditLink = ( section ) => ( section !== 'gutenberg-components' );
const getReadmeFilePath( section, example ) {
switch( section ) {
case 'design':
return `/client/components/${ example.props.readmeFilePath }/README.md`;
case 'gutenberg-components';
return `/node_modules/@wordpress/components/src/${ example.props.readmeFilePath }/README.md`;
default:
return `/client/${ section }/${ example.props.readmeFilePath }/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.
Agreed!
client/devdocs/controller.js
Outdated
@@ -163,6 +163,14 @@ const devdocs = { | |||
next(); | |||
}, | |||
|
|||
// Gutenberg C |
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.
😄 Don't forget to finish your comment
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.
Oops! 😅 You have not seen anything 🤫
@@ -8,14 +8,15 @@ import classNames from 'classnames'; | |||
import PropTypes from 'prop-types'; | |||
import { LiveProvider, LiveEditor, LiveError, LivePreview } from 'react-live'; |
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.
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import Button from 'components/button'; | ||
import ClipboardButton from 'components/forms/clipboard-button'; | ||
import DocsExampleWrapper from 'devdocs/docs-example/wrapper'; | ||
import * as playgroundScope from 'devdocs/design/playground-scope'; | ||
import * as playgroundScopeForComponents from 'devdocs/design/playground-scope'; |
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.
🤔 not sure we can get around not importing both sets.
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.
What about this?
const scope = 'gutenberg-components' === section ? require('@wordpress/components') : require('devdocs/design/playground-scope');
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 depends on where we put the requires. For example think of visiting http://calypso.localhost:3000/devdocs/design, then clicking on Gutenberg Components without a browser refresh.
Maybe we could separate the two into two components and then async load?
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.
require
loads modules on-demand, so it shouldn't cause any issues when switching from one page to another
let scope = playgroundScopeForComponents; | ||
if ( 'gutenberg-components' === this.props.section ) { | ||
scope = playgroundScopeForGutenbergComponents; | ||
} |
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.
How about a ternary to avoid the let?
const scope = 'gutenberg-components' === this.props.section ? playgroundScopeForGutenbergComponents : playgroundScopeForComponents;
webpack.config.js
Outdated
} else { | ||
// jquery is only needed in the build for the desktop app | ||
// see electron bug: https://github.com/atom/electron/issues/254 | ||
webpackConfig.externals.push( 'jquery' ); |
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.
Ah we'll need to make sure we remove this before change before merge. This is still used by Desktop
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.
Is it still there? I hoped you would refactor paygate
to be jQuery
free :D
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.
@astralbodies did anything change with regards to the jQuery include? Pretty sure we still need this, right?
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 required by @wordpress/api-fetch
, which is a dependency of @wordpress/components
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 need to fix it sooner than later! 😅
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 required by @wordpress/api-fetch, which is a dependency of @wordpress/components
I think we should get rid of the jQuery dependency here. The issue in this case is that jQuery in core triggers events on its own events layer, which we can't listen to with native addEventListener
s. @gziolo had a good idea: use wp.hooks
to relay the jQuery event into a native event, which can then be listened to by api-fetch
. Possibly by adding some code to do this as an inline script: https://github.com/WordPress/gutenberg/blob/master/lib/client-assets.php#L154-L161
I think it would also be a good idea to audit what we're depending on jQuery for in Gutenberg in general, and see what can be factored 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.
Thanks for the suggestion! I have applied it in WordPress/gutenberg#8311, so as soon as that is merged and published to npm I'll remove the jQuery
dependency.
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.
@ehg I have reverted the change and jQuery is not needed now in the bundle. Is this ready to go now?
Overall this looks very promising! Let's see what other folks like @ehg and @mtias think! Another thing to check is to verify that bundle sizes don't change unexpectedly in http://iscalypsofastyet.com/branch?branch=add/devdocs-gutenberg-components |
@@ -0,0 +1,47 @@ | |||
/** @format */ |
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 add this code in Gutenberg?
It might be more beneficial to improve docs and add examples inside Gutenberg directly rather than keep them in two places. We can even keep those examples inside README
and extract them and auto-build components inside Calypso.
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.
that would be really nice!
Screens look sweet 👍 |
f5d7d5f
to
d91d18c
Compare
Thanks for this suggestion @gwwar. From what I'm understanding from these tips, I'd say everything is normal. Most of the chunks that have been increased are async loaded. The most relevant change probably is that the vendors build has been increased by 10.9% after including jQuery. |
All the latest changes published to npm:
This includes #8267 and another one which removes |
d91d18c
to
297d91c
Compare
Adding helper functions for getting the readme file path and knowing whether to show the edit in GitHub link
Finishing uncompleted comment
Loading only the one needed scope for in the component playground
Adding tests
Getting back jQuery
Getting back jQuery
Adding comment to make clear jQuery is needed temporarily
297d91c
to
894a537
Compare
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.
assets/stylesheets/_vendor.scss
Outdated
@@ -2,6 +2,7 @@ | |||
@import '../../node_modules/react-virtualized/styles'; | |||
@import '../../node_modules/draft-js/dist/Draft'; | |||
@import '../../node_modules/notifications-panel/src/boot/stylesheets/style'; | |||
@import '../../node_modules/@wordpress/components/build-style/style'; |
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.
Hmm, actually any ideas from folks on how to best audit that we're not colliding with existing selectors? Maybe it'd be safer to pull this in devdocs.scss
?
Any opinions here @Automattic/team-calypso ?
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.
Maybe it'd be safer to pull this in devdocs.scss ?
Let's do this for the moment, we're still figuring out SASS imports.
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.
Done :)
It looks good, I don't have any comments specific for WordPress packages. |
Other than the issue on where to import the SASS, I think we can land this. Nice work. |
First task for exposing the Gutenberg components in Calypso and part of #26180.
It creates a new section in the DevDocs that displays the collection of components available in Gutenberg, with documentation explaining how to use them.
Right now, the collection only features the
Button
component (rest of them will be included in the next PR).Testing Instructions
/devdocs
/devdocs/gutenberg-components
and a page with a short intro and a collection of component appearsButton
componentsButton
link@wordpress/components
package for theButton
component (https://github.com/WordPress/gutenberg/blob/master/packages/components/src/button/README.md)Screenshots