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

Proof of concept: render a Gutenberg component in Calypso DevDocs #26088

Closed
wants to merge 23 commits into from

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Jul 16, 2018

DO NOT MERGE

Work in progress

Summary

The next step of the current work for integrating the Gutenberg blocks in Calypso is to develop a proof on concept prototyping what a single Gutenberg block should look like in the new "Gutenberg Blocks" section in the DevDocs.

As the Gutenberg core-blocks package is not ready yet (WordPress/gutenberg#3955), this version of the POC will use the @wordpress/components package in order to move forward and identify potential blockers.

Notes

Tasks

  • Declare @wordpress/components as dependency
  • Add documentation for the Button Gutenberg component
  • Include the Button Gutenberg component in the Playground section
  • Check there are no issues with the @wordpress/compose, @wordpress/deprecated and @wordpress/dom sub deps.

Instructions

Blockers / Problems

screen shot 2018-07-16 at 17 27 40

  • There is a SCSS file in one of the Gutenberg components (scroll-lock) that doesn't match the expected pattern for the styles (it should be style.scss instead of index.scss and it should be imported in packages/components/src/styles.scss instead of in packages/components/src/scroll-block/index.js). This is causing an error I have not been able to solve with any work-around, as I think it should be fixed on Gutenberg Solved in Packages: Move the components module partially to the packages folder WordPress/gutenberg#7640

screen shot 2018-07-16 at 17 27 57

screen shot 2018-07-16 at 19 41 57

  • @babel/[email protected] is required by the @wordpress/components. This can be be fixed either by updating Babel in Calypso or by declaring a @babel/runtine dependency in @wordpress/components. Any project using @wordpress/components will have the same issue if @babel/runtime is not installed or if the installed version doesn’t match the one used during the package build process. It should be added to the @wordpress/components package if we don't want to rely on the installed dependencies on a project using it, so the package works out-of-the-box. Solved in Add @babel/runtime as a dependency to @wordpress/components WordPress/gutenberg#8057

@mmtr mmtr added [Status] In Progress DevDocs DO NOT MERGE [Goal] Gutenberg Working towards full integration with Gutenberg labels Jul 16, 2018
@mmtr mmtr self-assigned this Jul 16, 2018
@matticbot
Copy link
Contributor

@mmtr mmtr requested review from ehg, gziolo and gwwar July 16, 2018 16:07
@gziolo
Copy link
Member

gziolo commented Jul 16, 2018

@wordpress/components shouldn't depend on @wordpress/utils anymore. I merged 2 PRs which contributed to that earlier today. You just need to rebase the Gutenberg branch against the latest master :)

@gziolo gziolo requested a review from youknowriad July 16, 2018 16:11
@gziolo
Copy link
Member

gziolo commented Jul 16, 2018

  • There is a SCSS file in one of the Gutenberg components (scroll-lock) that doesn't match the expected pattern for the styles (it should be style.scss instead of index.scss and it should be imported in packages/components/src/styles.scss instead of in packages/components/src/scroll-block/index.js). This is causing an error I have not been able to solve with any work-around, as I think it should be fixed on Gutenberg

screen shot 2018-07-16 at 17 27 57

@youknowriad, any ideas why it happens? Did we simply miss it?

@youknowriad
Copy link
Contributor

  • There is a SCSS file in one of the Gutenberg components (scroll-lock) that doesn't match the expected pattern for the styles (it should be style.scss instead of index.scss and it should be imported in packages/components/src/styles.scss instead of in packages/components/src/scroll-block/index.js). This is causing an error I have not been able to solve with any work-around, as I think it should be fixed on Gutenberg

@mmtr @gziolo Good catch, the WordPress/gutenberg#7640 has been updated and should be ready for a final review before merging which hopefully will simplify the work being done here.

@mmtr
Copy link
Member Author

mmtr commented Jul 16, 2018

Thanks both @gziolo @youknowriad! I'm no longer getting those errors :)

@mmtr
Copy link
Member Author

mmtr commented Jul 17, 2018

@gziolo I'm trying to compile the @wordpress/components SCSS files in order to customize the styles using the SCSS variables defined in Calypso, but I'm getting this error:

screen shot 2018-07-17 at 11 55 25

Not sure why the react-datepicker package is not in the node_modules of @wordpress/components despite of being defined as dependency in the package.json file.

screen shot 2018-07-17 at 12 08 09

Did I skip any step for building the packages locally?

@youknowriad
Copy link
Contributor

I'm trying to compile the @wordpress/components SCSS files in order to customize the styles using the SCSS variables defined in Calypso, but I'm getting this error:

While I understand the reasoning behind this change, I have some concerns about it. Sass files are not considered deliverables in our npm modules. Gutenberg is using them today but once we ship npm modules, I doubt you'll have access to them. Also, it's just an implementation detail, we could consider switching to CSS in JS for example in which case the Sass files won't exist anymore.

I tend to think we should reconsider this and only override styles with CSS. Sass files and sass variables are not an officially supported API in our npm packages.

@mmtr
Copy link
Member Author

mmtr commented Jul 17, 2018

Thanks @youknowriad, I see your point.

But that would depend on how we want to deliver the @wordpress/components package. Do we want to offer flexibility so anyone can easily customize them in order to match a specific visual identity? If yes, I would reconsider to provide some mechanisms for doing that. If not, the current CSS approach will be enough. I guess this flexibility feature is not a high priority now.

So the question now is, do we want to customize the Gutenberg components in Calypso in order to match the Calypso visual identity or is ok to leave them as originally defined by Gutenberg. What do you think @ehg?

@mmtr mmtr mentioned this pull request Jul 17, 2018
9 tasks
@gziolo
Copy link
Member

gziolo commented Jul 17, 2018

@mmtr, we are merging @wordpress/components into Gutenberg master later today. We should publish it to npm tomorrow (hopefully - depends on a few cleanup PRs which are in review).

@mmtr
Copy link
Member Author

mmtr commented Jul 17, 2018

Thanks for the update @gziolo! Do you you have any plan for start working on the core-blocks package?

@gziolo
Copy link
Member

gziolo commented Jul 17, 2018

core-blocks depends on components, editor and blocks, so it's far on the list :)

@gziolo
Copy link
Member

gziolo commented Jul 18, 2018

For the record, @wordpress/components was published to npm and is now referenced in this PR 🎉

@mmtr
Copy link
Member Author

mmtr commented Jul 18, 2018

I have just updated the description describing a new issue we have related to the @babel/runtime package

@mmtr mmtr mentioned this pull request Jul 19, 2018
27 tasks
@gwwar
Copy link
Contributor

gwwar commented Jul 19, 2018

✨ This also ties in nicely as part of #26180

@gwwar
Copy link
Contributor

gwwar commented Jul 19, 2018

@gziolo @youknowriad

@babel/[email protected] is required by the @wordpress/components

Since components is a library, would it be possible to move dependencies needed to build the js/css bundle to devDependencies? Once we publish to npm, the parent application doesn't need to build additional files from this module, right?

See #26088 (comment)

} 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' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Does @wordpress/components still depend on jQuery?

Copy link
Member Author

@mmtr mmtr Jul 19, 2018

Choose a reason for hiding this comment

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

yep. @wordpress/components needs @wordpress/api-fetch, and this one needs jQuery

Copy link
Contributor

Choose a reason for hiding this comment

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

Complete aside, but if you type y on a GitHub file view, it'll give you a permanent link to the file like so: https://github.com/WordPress/gutenberg/blob/348e463b7cbdd246cf0744f6fa1459fefd0f37a7/packages/components/package.json#L28 (so the highlighted line remains the same if folks click on it in the future)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I totally misread the summary of WordPress/gutenberg#7566, they're starting to move away from jQuery but it's still a dep

@gwwar
Copy link
Contributor

gwwar commented Jul 19, 2018

Hmm, taking a quick look, I had a little trouble seeing where the babel/[email protected] is coming from, I'll 🔍a bit later, and will make a PR if I find it.

I had a chat with @blowery and @mmtr, and the issue is that we don't declare @babel/runtime as a dependency in the package, but we do need it. Internally in the published package, we have calls like below, that are used to help polyfill calls, without adding a lot of additional code if the consuming parent is using babel/runtime as well.

var _objectWithoutProperties2 = _interopRequireDefault(require("@babel/runtime/helpers/objectWithoutProperties"));

Next steps to remove this blocker:

@gziolo
Copy link
Member

gziolo commented Jul 20, 2018

We landed WordPress/gutenberg#8057 and published updated packages to npm. You shouldn't need to provide babel/runtime as a dependency anymore. We still should look into how optimize distributed packages as it probably is far from ideal 😅

@mmtr
Copy link
Member Author

mmtr commented Jul 23, 2018

everything looks good so far with the @wordpress/components package, so I'm closing this PR as we don't want to expose the Gutenberg components in Calypso for the moment.

the goal of this was to identify potential blockers and I think we're done with that :)

thanks everyone involved!

@mmtr mmtr closed this Jul 23, 2018
@mmtr mmtr deleted the try/gutenberg-devdocs-poc-component branch August 9, 2018 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DevDocs DO NOT MERGE [Goal] Gutenberg Working towards full integration with Gutenberg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants