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 for #264, repeatable groups with a WYSIWYG editor, take 2 #693

Closed
wants to merge 1 commit into from

Conversation

johnsonpaul1014
Copy link
Contributor

This is a second attempt at a pull request to fix the repeatable WYSIWYG editor fields. It adds in a fix to make sure that editors aren't attempted to be destroyed before they actually exist. I have been using this in production for a bit now with no more issues. There may be a better way to integrate this with existing CMB code, but I was able to pull this out of some other logic I had to make WYSIWYG editors.

@matgargano
Copy link

👍 getting this in would be awesome

@jtsternberg
Copy link
Member

Not ignoring, just busy. We'll be reviewing soon.

@jtsternberg
Copy link
Member

Hmm, you don't actually ever enqueue the wysiwyg.js. Is there a reason for that?

@jtsternberg
Copy link
Member

I see, you've concatenated it, but their is no solution if SCRIPT_DEBUG is enabled. I'm reviewing now, so please don't make any updates.

@johnsonpaul1014
Copy link
Contributor Author

That's a good point. It probably makes more sense to enqueue it with the main script having a dependency on it, so a local wysiwyg variable could be used instead of window.wysiwyg. I was just trying to eliminate the extra server request.

This was referenced Aug 1, 2016
jtsternberg added a commit that referenced this pull request Aug 1, 2016
Fixes #26, fixes #99, fixes #260, fixes #264, fixes #356,
fixes #431, fixes #462, fixes #657, fixes #693
@jtsternberg
Copy link
Member

@johnsonpaul1014 Thanks a ton for getting the ball rolling. I've merged your branch in and used it as a (great) starting point. I had to re-think things to make it a bit more performant (there was no reason to destroy/reinit all instances when shifting rows), and fix a few bugs (removing a group high up in the stack resulted in a bad initiation when new rows were added). I also updated the script-loader to properly enqueue the file separately if SCRIPT_DEBUG is enabled. Let me know if you have any questions, and I would love it if you (and anyone listening) would test the new PR before I merge it in to trunk.

@jtsternberg jtsternberg closed this Aug 1, 2016
@jtsternberg
Copy link
Member

This is now in trunk and will be in the next release.

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