-
Notifications
You must be signed in to change notification settings - Fork 756
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
Feature/4101 update ck editor version #4383
Feature/4101 update ck editor version #4383
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This will certainly need some real world testing, but all of the changes look good.
Once #4374 is merged, I have a branch where I've rebased this PR on those changes. Thanks for the very clean commits, it made the rebase very straightforward :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this editor provider!
5882cab
to
d42c1f3
Compare
FYI, I've pushed the rebased commits to your branch since #4374 was merged. |
While testing some more today, I noticed 2 plugins and a skin where missing from the updated CKE:
the 3 commits (just now) add these to the CKE folder. |
Another two commits needed after testing. Link popup didn't seem to work.
|
@@ -2,6 +2,8 @@ | |||
// The .NET Foundation licenses this file to you under the MIT license. | |||
// See the LICENSE file in the project root for more information | |||
|
|||
using DNNConnect.CKEditorProvider.Constants; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) Can this get moved inside the namespace
declaration, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll fix that.
I noticed changes like this recently. Do you know the benefits of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's primarily a style rule, but it does have some minor benefits (described in the StyleCop rule docs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, thanks.
Thanks! |
Closes #4101
Summary
I updated CKEditor to version 4.15.1 and made the required/appropriate changes for that: