Skip to content

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

Merged
youknowriad merged 2 commits intomasterfrom
update/import-wp-modules
Jun 21, 2017
Merged

Framework: Use imports for WordPress modules instead of the wp global#1332
youknowriad merged 2 commits intomasterfrom
update/import-wp-modules

Conversation

@youknowriad
Copy link
Copy Markdown
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 aduth and nylen 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
Copy Markdown
Member

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

@ellatrix
Copy link
Copy Markdown
Member

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

@youknowriad
Copy link
Copy Markdown
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
Copy Markdown
Member

ellatrix commented Jun 21, 2017

Hm, ran it and it passes?

@ellatrix
Copy link
Copy Markdown
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
Copy Markdown
Member

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

@ellatrix
Copy link
Copy Markdown
Member

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

@youknowriad
Copy link
Copy Markdown
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
Copy Markdown
Member

Oh, this has been attempted before: #742.

@ellatrix
Copy link
Copy Markdown
Member

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

@youknowriad
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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