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

Fix WYSIWYG metaboxes saving #12363

Closed
wants to merge 1 commit into from
Closed

Fix WYSIWYG metaboxes saving #12363

wants to merge 1 commit into from

Conversation

youknowriad
Copy link
Contributor

closes #7176

This is the exact same fix I've applied before in #8762 but in my testing this time it works properly and fixes the issue consistently. I tested using @danielbachhuber's snippet here #7176 (comment)

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended [Feature] Meta Boxes A draggable box shown on the post editing screen labels Nov 27, 2018
@youknowriad youknowriad added this to the 4.6 milestone Nov 27, 2018
@youknowriad youknowriad self-assigned this Nov 27, 2018
@youknowriad youknowriad requested a review from a team November 27, 2018 14:51
@danielbachhuber
Copy link
Member

but in my testing this time it works properly and fixes the issue consistently

Were you able to finally reproduce the issue consistently?

@youknowriad
Copy link
Contributor Author

Yes, I was. I'd appreciate other people testing too because I don't really understand what happens the first time we tried this :)

@youknowriad
Copy link
Contributor Author

After some testing in #7176 It looks like this PR fixes the issue if you are on 5.0 RC. I don't have the full explanation yet. Maybe it has something to do with how TinyMCE initialization work but also would happy to land it as is. As 5.0RC is our most important target.

@youknowriad youknowriad modified the milestones: 4.6, 4.7 Nov 29, 2018
@ideadude
Copy link
Contributor

ideadude commented Dec 3, 2018

When I checkout the fix/metabox-saving branch and reinstall/build, this does NOT fix the issue for me. I'm on 4.9.9-alpha-43656 via the beta tester plugin. Does this count as 5.0 RC?

The CMB2 fix does work for me. I can work on a PR that merges the two approaches.

@ideadude
Copy link
Contributor

ideadude commented Dec 3, 2018

Running tinymce.triggerSave(); doesn't work for my test.

When I looked into this earlier, it seemed like WP was adding some kind of check that would skip the save if the main tinymce editor was on the text tab, but this would skip all of the editors on the screen. Something like that.

When I swap the triggerSave line for the relevant code from CMB2 fix (similar to code I shared on another issue somewhere), it works:

for (var i = 0; i < window.tinymce.editors.length; i++) {
  window.tinymce.editors[i].save();
}

Feel free to update your PR or let me know if it would help if I did a separate PR with this version. Whatever will get the fix done fastest.

@youknowriad
Copy link
Contributor Author

youknowriad commented Dec 3, 2018

Please if you have some time, let's open a separate PR :). Thanks for your help figuring this out.

@ideadude
Copy link
Contributor

ideadude commented Dec 3, 2018

Working on it. Coming in soon. I'll link it from here.

@ideadude
Copy link
Contributor

ideadude commented Dec 3, 2018

Here is my alternative PR: #12568

Other testers would be appreciated.

Let me know if you need anything else from me.

@youknowriad
Copy link
Contributor Author

closing in favor of #12568

@youknowriad youknowriad deleted the fix/metabox-saving branch December 9, 2018 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Meta Boxes A draggable box shown on the post editing screen [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call tinyMCE.triggerSave() before sending metabox data
3 participants