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

Upload Widget crash #967

Closed
wants to merge 1 commit into from
Closed

Conversation

MaksimMakarevich
Copy link
Contributor

@MaksimMakarevich MaksimMakarevich commented Sep 27, 2017

What is the purpose of this pull request?

Bug fix

What changes did you make?

Added extra condition

Fix for #966

@mlewand mlewand added the review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer. label Sep 27, 2017
@@ -251,8 +251,9 @@

loader.on( 'update', function( evt ) {
// Abort if widget was removed.
if ( !widget.wrapper || !widget.wrapper.getParent() ) {
if ( !editor.editable().find( '[data-cke-upload-id="' + id + '"]' ).count() ) {
if (!widget.wrapper || !widget.wrapper.getParent()) {
Copy link
Contributor

@msamsel msamsel Sep 28, 2017

Choose a reason for hiding this comment

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

Please adapt your code style to look like in other part of CKEdtor. More information about styling you can find here. It's helpful to install githooks which checks styling when you perform a commit. Information about githooks you can find here

Copy link
Contributor

@msamsel msamsel left a comment

Choose a reason for hiding this comment

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

Hi,
thank you for your pull request. There is still few improvements, which might be done, before your pull request will be possible to merge. General information about contributing you can find here in our guide. Those things currently are missing in your pull request:

  • All changes in CKEditor requires tests. Please provide proper unit and manual tests. More information about tests you can find under this link
  • Please also remember to sign CLA (it should be send on your email). Without it we cannot implement any changes made by you.

@mlewand mlewand self-requested a review July 19, 2018 21:35
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

The code was OK, but the tests were missing.

I have added the tests, and adjusted the code style. All of this pushed to t/966 which is also rebased onto latest master branch.

@mlewand
Copy link
Contributor

mlewand commented Jul 19, 2018

This PR was merged manually with 33d301f.

Thanks for the input, and sorry for the delay it's been a busy period for us 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants