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

Framework: Use imports for WordPress modules instead of the wp global #1332

Merged
merged 2 commits into from
Jun 21, 2017

Conversation

youknowriad
Copy link
Contributor

part of #1205

I would appreciate a quick review for this to avoid many conflicts :)

@youknowriad youknowriad added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jun 21, 2017
@youknowriad youknowriad self-assigned this Jun 21, 2017
@youknowriad youknowriad requested review from nylen and aduth June 21, 2017 11:38
@youknowriad youknowriad removed the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jun 21, 2017
@ellatrix
Copy link
Member

Love it. Do you think we can remove the wp global from eslint too, for better testing?

@ellatrix
Copy link
Member

If we need it anywhere else for external stuff, we can force window.wp.

@youknowriad
Copy link
Contributor Author

@iseulde Not against it, but maybe there're too much wp usage left for this. (All the wp.api, wp.media usages)

@ellatrix
Copy link
Member

ellatrix commented Jun 21, 2017

Hm, ran it and it passes?

@ellatrix
Copy link
Member

Aha forgot it inherits from other config. Will remove commit

@ellatrix ellatrix force-pushed the update/import-wp-modules branch from 369681d to a9ad2f0 Compare June 21, 2017 12:08
@ellatrix
Copy link
Member

In that case, can we change that with Webpack? It seems strange to still have both after this PR.

@ellatrix
Copy link
Member

Mostly asking because I'm not sure how we'll prevent it from being introduced again unknowingly.

@youknowriad
Copy link
Contributor Author

In that case, can we change that with Webpack?

I'm trying but failing to define these modules in webpack. It seems to accept wp as an external module but not wp.media. Any idea?

@ellatrix
Copy link
Member

Oh, this has been attempted before: #742.

@ellatrix
Copy link
Member

Yeah, wp.media itself is a function... :)

@youknowriad
Copy link
Contributor Author

Yeah, I'm thinking we'd need to add a namespace to resolve this import { media } from '@wordpress';

Anyway, I'm keen to merge this as a first step and see how to solve the external wp modules issue.

@@ -19,7 +20,7 @@ import './style.scss';
import FormatToolbar from './format-toolbar';
import TinyMCE from './tinymce';

function createElement( type, props, ...children ) {
function createTinymceElement( type, props, ...children ) {
Copy link
Member

Choose a reason for hiding this comment

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

Capitalization: We should probably prefer TinyMCE, createTinyMCEElement

@youknowriad youknowriad force-pushed the update/import-wp-modules branch from a9ad2f0 to ce6bde0 Compare June 21, 2017 14:10
@youknowriad youknowriad merged commit 93ad7a6 into master Jun 21, 2017
@youknowriad youknowriad deleted the update/import-wp-modules branch June 21, 2017 14:16
@youknowriad youknowriad mentioned this pull request Jun 22, 2017
3 tasks
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