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

Jump to bottom

Packages: Extract the editor package #8081

Merged
merged 3 commits into from
Jul 27, 2018
Merged

Packages: Extract the editor package #8081

merged 3 commits into from
Jul 27, 2018

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jul 20, 2018 •
edited
Loading

Description

Part of #3955.

This PR extracts new @wordpress/editor package.

Remaining

How has this been tested?

Make sure all tests pass

@youknowriad youknowriad self-assigned this Jul 20, 2018
gziolo
gziolo reviewed Jul 23, 2018
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

So far so good. All changes look good. I like that you replaced occurrences of registerCoreBlocks with local definitions of the required blocks.

packages/dom/package.json
@@ -22,8 +22,7 @@
"dependencies": {
"@babel/runtime": "^7.0.0-beta.52",
"element-closest": "^2.0.2",
"lodash": "^4.17.10",
"tinymce": "^4.7.2"
"lodash": "^4.17.10"
Copy link
Member

Choose a reason for hiding this comment

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

Nice, to see tinymce isn't required by dom anymore.

@youknowriad youknowriad mentioned this pull request Jul 23, 2018
Merged
@youknowriad youknowriad force-pushed the add/editor-package branch 2 times, most recently from 251778e to b3d166e Compare July 26, 2018 11:26
@youknowriad youknowriad changed the title WIP: Packages: Extract the editor package Packages: Extract the editor package Jul 26, 2018
@youknowriad youknowriad added the npm Packages Related to npm packages label Jul 26, 2018
@youknowriad youknowriad force-pushed the add/editor-package branch 2 times, most recently from aec4a4c to e9de074 Compare July 26, 2018 11:48
youknowriad
youknowriad commented Jul 26, 2018
View reviewed changes
packages/editor/package.json
"lodash": "^4.17.10",
"memize": "^1.0.5",
"querystring": "^0.2.0",
"querystringify": "^1.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use multiple querystring libraries, we should settle on one. I'm leaving this for a later PR though.

Copy link
Contributor

Choose a reason for hiding this comment

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

And Gutenberg itself also requires http-build-query to build PHP-compatible query strings (because PHP has to be special), which adds to the query string fun :)

@gziolo gziolo requested review from hypest, gwwar and Copons July 26, 2018 19:32
@gwwar gwwar requested a review from vindl July 26, 2018 19:32
@gziolo
Copy link
Member

gziolo commented Jul 26, 2018 •
edited
Loading

@hypest - we are changing the way styles are loaded, we no longer use import 'style.scss'; and Webpack to do it. I noticed that there is one override coming from React Native. We will have to find a new way to build CSS styles for mobile. Let's open a follow up for it. There is now style.scss file in the src folder of the editor package, so if RN build is able to load overrides then we will only have to generate CSS output.

At the moment it might still work since Riad left import styles from './style.scss'; in the *.native.js file.

gziolo
gziolo reviewed Jul 26, 2018
View reviewed changes
packages/blocks/package.json
@@ -32,7 +32,6 @@
"@wordpress/i18n": "file:../i18n",
"@wordpress/is-shallow-equal": "file:../is-shallow-equal",
"@wordpress/shortcode": "file:../shortcode",
"dom-react": "^2.2.1",
Copy link
Member

Choose a reason for hiding this comment

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

When checking listed deps I discovered it is no longer used in blocks 🤷‍♂️

@gziolo
Copy link
Member

gziolo commented Jul 26, 2018

@youknowriad, I added a few very small tweaks based on the things I found during review. I also executed npm run docs:build to regenerate docs after moving editor to packages folder.

@gziolo gziolo added this to the 3.4 milestone Jul 26, 2018
gziolo
gziolo reviewed Jul 26, 2018
View reviewed changes
packages/editor/src/components/colors/with-colors-deprecated.js Outdated
@@ -0,0 +1,92 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

I think this file was removed. It looks like a merge issue.

gziolo
gziolo approved these changes Jul 26, 2018
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Looks great in my opinion. It also tests well.

There is still one file with-colors-deprecated.js which most likely should be removed. I think @jorgefilipecosta can confirm.

@gziolo
Copy link
Member

gziolo commented Jul 26, 2018

Travis failed again on one of the tests:

FAIL test/e2e/specs/preview.test.js (6.099s)

If I see it again on Monday, I will disable this test ...

@youknowriad youknowriad mentioned this pull request Jul 27, 2018
Closed
@youknowriad youknowriad merged commit 94f5489 into master Jul 27, 2018
@youknowriad youknowriad deleted the add/editor-package branch July 27, 2018 14:35
@Shelob9 Shelob9 mentioned this pull request Aug 1, 2018
Closed
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisvanpatten chrisvanpatten chrisvanpatten left review comments

@gziolo gziolo gziolo approved these changes

@hypest hypest Awaiting requested review from hypest

@gwwar gwwar Awaiting requested review from gwwar

@Copons Copons Awaiting requested review from Copons

@vindl vindl Awaiting requested review from vindl

Assignees

@youknowriad youknowriad

Labels
npm Packages Related to npm packages
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
3 participants
@youknowriad @gziolo @chrisvanpatten