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

Bugfix - Tableresize not working in maximized mode with divarea #3694

Merged
merged 6 commits into from
Jun 5, 2020

Conversation

arpi68
Copy link
Contributor

@arpi68 arpi68 commented Nov 29, 2019

What is the purpose of this pull request?

Bug fix - Tableresize not working in maximized mode when using divarea

What is the proposed changelog entry for this pull request?

Tableresize not working in maximized mode when using divarea

What changes did you make?

z-index of resizer in maximized mode too low.
Set z-index of resizer to 10000

Which issues your PR resolves?

Closes #1959.
Closes #909.

@f1ames
Copy link
Contributor

f1ames commented May 4, 2020

Hello @arpi68,

thanks for contributing! Your pull request is missing tests, see our [Contributing Code](http://docs.ckeditor.com/#!/guide/dev_contributing_code) guide for more details.

For such fixes, manual tests should be enough since we are not able to check the resizes visibility reliably with unit tests. Would you be able to provide manual tests fro this PR?

@hub33k hub33k self-assigned this May 25, 2020
@hub33k hub33k requested a review from f1ames May 27, 2020 11:59
@hub33k
Copy link
Contributor

hub33k commented May 27, 2020

@f1ames shouldn't we merge this PR into master?

@f1ames f1ames assigned f1ames and unassigned hub33k May 28, 2020
@f1ames f1ames changed the base branch from major to master May 28, 2020 11:21
@f1ames
Copy link
Contributor

f1ames commented May 28, 2020

@hub33k if you are asking about target branch then yes, since it is a bugfix it should be targeted to master branch, good catch 👍

I already retargeted it.

@f1ames f1ames force-pushed the arpi68-tableresizepatch-1 branch from b1217c9 to 9e13a59 Compare May 28, 2020 11:38
@f1ames
Copy link
Contributor

f1ames commented May 28, 2020

Rebased onto latest master.

tests/plugins/tableresize/manual/maximizeddivarea.md Outdated Show resolved Hide resolved
tests/plugins/tableresize/manual/maximizeddivarea.md Outdated Show resolved Hide resolved
</div>

<script>
CKEDITOR.replace( 'editor' );
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to have all 3 editor types available in this test, so:

  • Classic one (you may use removePlugins: divarea to make sure it's iframe based),
  • divarea - already present,
  • Inline one - using CKEDITOR.inline.

This way we may catch any strange behaviors in any editor types (unlikely to happen here, but it's a good practice anyway to have coverage for all editor types).

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, there is a slight issue with inline editor since maximze plugin is disabled there (as it doesn't make much sense there).

So maybe let's leave classic and divarea editor as is and for inline editor lets go with some absolute positioned wrapper with high z-index (so we will have also case mentioned in #909) covered. It should be probably a separate manual test then.

@hub33k hub33k self-assigned this May 28, 2020
@hub33k hub33k requested a review from f1ames June 2, 2020 11:56
@f1ames f1ames assigned f1ames and unassigned hub33k Jun 3, 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 👍 Just one thing regarding manual test - see #3694 (comment).

@hub33k hub33k requested a review from f1ames June 4, 2020 11:55
@f1ames f1ames self-assigned this Jun 5, 2020
@f1ames f1ames force-pushed the arpi68-tableresizepatch-1 branch from 0a0a479 to 5116ec2 Compare June 5, 2020 15:24
@f1ames
Copy link
Contributor

f1ames commented Jun 5, 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 👍

@f1ames f1ames merged commit e3be5c1 into ckeditor:master Jun 5, 2020
@f1ames
Copy link
Contributor

f1ames commented Jun 5, 2020

@arpi68 Thanks again for contributing to CKEditor 4 🎉👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants