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: Autogrow plugin causing width of editor's container to be set to 0px #4307

Merged
merged 14 commits into from
Oct 8, 2020

Conversation

hub33k
Copy link
Contributor

@hub33k hub33k commented Sep 30, 2020

What is the purpose of this pull request?

Bug fix

Does your PR contain necessary tests?

All patches that change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

  • Unit tests
  • Manual tests

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

  • PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#4286](https://github.com/ckeditor/ckeditor4/issues/4286): validate if width is valid value in editor.resize function.

What changes did you make?

Check if width has valid value.

Which issues does your PR resolve?

Closes #4286.

@hub33k hub33k marked this pull request as ready for review October 1, 2020 10:49
core/creators/themedui.js Outdated Show resolved Hide resolved
tests/plugins/autogrow/autogrowwidth.js Outdated Show resolved Hide resolved
tests/plugins/autogrow/autogrowwidth.js Outdated Show resolved Hide resolved
tests/plugins/autogrow/autogrowwidth.js Outdated Show resolved Hide resolved
tests/plugins/autogrow/autogrowwidth.js Outdated Show resolved Hide resolved
@grumd
Copy link

grumd commented Oct 2, 2020

We also encountered this issue in our project that's using CKEditor. Looking forward to see this merged soon.

Is there a way to use an older version of CKEditor in the online builder? (https://ckeditor.com/cke4/builder)
It only gives an option of using the latest version that's currently bugged.

@f1ames
Copy link
Contributor

f1ames commented Oct 2, 2020

Is there a way to use an older version of CKEditor in the online builder? (https://ckeditor.com/cke4/builder)
It only gives an option of using the latest version that's currently bugged.

@vokiel correct me if I'm wrong, but I don't think it's doable with online builder. @grumd you would need to create CKEditor 4 build "manually" (using ckeditor4-presets repo).

@grumd
Copy link

grumd commented Oct 2, 2020

@f1ames That repo seems to only support building the three default presets (basic/standard/full), but I found that it's possible to build ck4 code by checking out the branch/tag you need, and running /dev/builder/build.sh and using a custom /dev/builder/build-config.js. But I'd rather have the latest version so I'll wait a while for this PR first.

@vokiel
Copy link
Contributor

vokiel commented Oct 2, 2020

I don't think it's doable with online builder.

True, the online builder works with the latest version only.

@hub33k
Copy link
Contributor Author

hub33k commented Oct 2, 2020

@grumd you can create customized presets by creating two files: presets/$1-build-config.js & presets/$1-ckeditor-config.js (replace $1 with preset name) - ckeditor/ckeditor4-presets@2e89eb0.

@hub33k hub33k requested a review from f1ames October 6, 2020 18:46
@f1ames f1ames self-assigned this Oct 7, 2020
Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Only some polishing in unit tests needed - I don't see a reason to keep both unit tests in separate files so it will be good to merge them into one. See also review comments.

tests/plugins/autogrow/autogrow.js Outdated Show resolved Hide resolved
tests/plugins/autogrow/autogrow.js Outdated Show resolved Hide resolved
@hub33k hub33k requested a review from f1ames October 7, 2020 12:44
@f1ames f1ames self-assigned this Oct 8, 2020
@f1ames
Copy link
Contributor

f1ames commented Oct 8, 2020

Rebased onto latest master.

Copy link
Contributor

@f1ames f1ames left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@grumd
Copy link

grumd commented Oct 8, 2020

Great work! Any ETA on when a new release may happen with this PR included?

@f1ames
Copy link
Contributor

f1ames commented Oct 8, 2020

@grumd no definite ETA so far, but we are targeting beginning of December.

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.

4 participants