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

Mailchimp Widget: avoid conflicts with jQuery UI #8547

Merged
merged 3 commits into from Jan 25, 2018
Merged

Mailchimp Widget: avoid conflicts with jQuery UI #8547

merged 3 commits into from Jan 25, 2018

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Jan 17, 2018 •
edited

Fixes #8546

Changes proposed in this Pull Request:

  • Set window.define.amd to undefined after window.define is set by MailChimp script

Testing instructions:

  • Create a mailchimp popup window widget, and open any page where it should work kick in. It should work, and both window.define and window.require should be null.

Proposed changelog entry for your changes:

  • Ideally we would set window.require and window.define to undefined after using them in MailChimp shortcode, but MailChimp script needs require and define functions to remain in the global scope. Scripts like jquery will not use define if window.define.amd is undefined so we can at least do that.

@adamziel
Fix issue Automattic#8546 - remove window.require and window.define f…
a0966bb 
…rom global scope after using a require definition
@adamziel adamziel requested a review from a team as a code owner January 17, 2018 15:14
@jeherve jeherve added [Feature] Extra Sidebar Widgets [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Pri] Normal [Feature] Shortcodes / Embeds labels Jan 17, 2018
@jeherve jeherve changed the title Fix issue #8546 - remove window.require and window.define from global… Mailchimp Widget: avoid conflicts with jQuery UI Jan 17, 2018
@adamziel
Fix issue Automattic#8546 - instead of removing window.require and wi…
c609360 
…ndow.define from global scope and thus breaking mailchimp script, set window.define.amd to fool just real amd modules.

Mailchimp script seem to expect usable definitions of require and define, once we remove them it breaks. Since the purpose of this commit is to fix scripts like jquery-ui that will resort to define() if define.amd is defined - it is enough to unset define.amd. This is a very ugly solution, but it works.
@zinigor
Copy link
Member

zinigor commented Jan 19, 2018

Makes sense, but it looks like your change is breaking some tests that require the script to match a certain string verbatim. Can you please take a look at these?

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jan 19, 2018
zinigor
zinigor approved these changes Jan 25, 2018
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Tests pass now, LGTM!

@zinigor zinigor removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jan 25, 2018
jeherve
jeherve approved these changes Jan 25, 2018
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Looking good. Merging.

@jeherve jeherve merged commit 5729d9d into Automattic:master Jan 25, 2018
jeherve added a commit that referenced this pull request Jan 25, 2018
@jeherve
Changelog: add #8547
298f9c2
zinigor pushed a commit that referenced this pull request Jan 30, 2018
@oskosk @zinigor
Changelog 5.8: running changelog (#8442)
1ee2c03 
* Changelog 5.8: create base for changelog.

* Update 5.8 release post link

* fix 5.8 release date

* Updates to plugin description

* Changelog: add #8499

* Changelog: add #8506

* Changelog: add #8509

* Changelog: add #8516

* Changelog: add #8517

* Changelog: add #8523

* Changelog: add #8547

* Changelog: add #8496

* Changelog: add #8584

* Changelog: add #8595

* Changelog: add #8445

* Changelog: add #8431

* Changelog: add #8284

* Changelog: add #8270

* Changelog: add #8124

* Changelog: add #8581

* Changelog: add #8463

* Changelog: add #8568 (#8646)

* Updates to testing list and changelog

* Changelog: add #8443

* Changelog: add #8459

* Changelog: add #8469

* Changelog: add #8464

* Changelog: add #8478 and #8479

* Changelog: add #8483

* Changelog: add #8488

* Changelog: add #8513

* Changelog: add #8555

* Changelog: add #8565

* Changelog: add #8601

* Changelog: add #8612

* Changelog: add first pass at Search items.

* Changelog: add more info to help test Search.

* Changelog: add #8144

* Changelog: add #8313

* Changelog: add #8419

* Changelog: add #8465

* Changelog: add #8515

* Changelog: add #8587

* Changelog: add #8591

* Changelog: add #8659

* Changelog: add #8661

* Changelog: add #8671

* Changelog: add 5.7.1 to archived changelog too.

* Reverted changes to readme, removed entry about backups.
jeherve added a commit that referenced this pull request Sep 6, 2018
@jeherve
Shortcodes: update Mailchimp shortcode to match new format.
3318225 
Fixes #10104

Mailchimp updated their newsletter embed code, and the old one does not work anymore.
This change matches the new format, and updates our test accordingly.

Note that this also reverts the changes introduced in #8547, since they do not appear necessary anymore. Mailchimp fixed that issue on their end.
@jeherve jeherve mentioned this pull request Sep 6, 2018
Merged
jeherve added a commit that referenced this pull request Sep 20, 2018
@jeherve
Shortcodes: update Mailchimp shortcode to match new format.
2fb58e1 
Fixes #10104

Mailchimp updated their newsletter embed code, and the old one does not work anymore.
This change matches the new format, and updates our test accordingly.

Note that this also reverts the changes introduced in #8547, since they do not appear necessary anymore. Mailchimp fixed that issue on their end.
kraftbj pushed a commit that referenced this pull request Sep 21, 2018
@jeherve @kraftbj
Shortcodes: update MailChimp shortcode to match new format. (#10105)
e572356 
* Shortcodes: update Mailchimp shortcode to match new format.

Fixes #10104

Mailchimp updated their newsletter embed code, and the old one does not work anymore.
This change matches the new format, and updates our test accordingly.

Note that this also reverts the changes introduced in #8547, since they do not appear necessary anymore. Mailchimp fixed that issue on their end.

* Update testing list to include steps for #10105
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zinigor zinigor zinigor approved these changes

@jeherve jeherve jeherve approved these changes

Assignees
No one assigned
Labels
[Feature] Extra Sidebar Widgets [Feature] Shortcodes / Embeds [Pri] Normal [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@adamziel @zinigor @jeherve @matticbot