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

Keep js on calendar to prevent conflicts with other plugins/themes. #239

Closed
wants to merge 1 commit into from

Conversation

cojennin
Copy link
Contributor

The following should solve this problem http://wordpress.org/support/topic/mail-poet-former-wysija-newsletter-conflict/ and other issues that might arise from Edit Flow js getting enqueued on the wrong pages.

@danielbachhuber
Copy link
Contributor

It would be good to start an abstraction here by possibly creating a helper method is_module_page() or similar

@cojennin
Copy link
Contributor Author

One thing I've noticed that I'd like to tackle in conjunction with this is take all globals Edit Flow creates and leverage wp_localize_script. Think we'll get some benefits from 1) namespacing 2) #241 If we decide to automate with jshint, jshint does not like undefined variables (which happens with the globals created). 3) Localization of js will likely be made easier if we implemented it in this fashion.

Regarding issue 2), I was envisioning the structure of the global js object being something like:

EF_Options = {
Common : { ...globals here... },
Settings : { ...globals here... },
Editorial_Comments : { ..globals here... },
Calendar : { ..globals here... }
...
}

That way we keep it nice and namespaced not just between Edit Flow and other plugins, but also between Edit Flow modules (without polluting the global namespace, and avoids having to add more globals to .jshintrc in the long run).

The only issue I see here is that wp_localize_script isn't really set up to handle this structure, so always good to hear some other ideas if folks have them.

@danielbachhuber
Copy link
Contributor

I'm down. Let's do this in tandem with #240, maybe for 0.9 if it ends up being a larger amount of work.

@cojennin
Copy link
Contributor Author

Closing in favor of #351

@cojennin cojennin closed this May 17, 2016
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.

2 participants