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

Detect if mention panel goes beyond window borders #4266

Merged
merged 55 commits into from
Dec 8, 2020
Merged

Detect if mention panel goes beyond window borders #4266

merged 55 commits into from
Dec 8, 2020

Conversation

hub33k
Copy link
Contributor

@hub33k hub33k commented Sep 8, 2020

What is the purpose of this pull request?

New feature

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?

* [#3582](https://github.com/ckeditor/ckeditor4/issues/3582): Introduced smart positioning of [Autocomplete](https://ckeditor.com/cke4/addon/autocomplete) panel used by [Mentions](https://ckeditor.com/cke4/addon/mentions) and [Emoji](https://ckeditor.com/cke4/addon/emoji) plugins.

What changes did you make?

When mention panel is about to goes beyond window borders it will open in proper position.

Which issues does your PR resolve?

Closes #3582.

@hub33k
Copy link
Contributor Author

hub33k commented Sep 8, 2020

Rebased onto latest major.

@hub33k
Copy link
Contributor Author

hub33k commented Sep 9, 2020

Rebased onto latest major.

@hub33k hub33k marked this pull request as ready for review September 10, 2020 13:27
@hub33k
Copy link
Contributor Author

hub33k commented Sep 10, 2020

I'm not entirely sure about the correctness of the unit test. I think it can be improved 🤔

@f1ames f1ames self-requested a review September 11, 2020 08:21
@f1ames f1ames self-assigned this Sep 11, 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.

Good start, but there are few more cases to consider (see review comments).

plugins/autocomplete/plugin.js Outdated Show resolved Hide resolved
plugins/autocomplete/plugin.js Outdated Show resolved Hide resolved
tests/plugins/autocomplete/manual/windowborder.md Outdated Show resolved Hide resolved
plugins/autocomplete/plugin.js Outdated Show resolved Hide resolved
@hub33k hub33k self-assigned this Sep 15, 2020
@github-actions
Copy link

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

@github-actions github-actions bot added the pr:stale 🗿 Old PR stale label. Currently "stale" label is used to mark both stale issues and PRs. label Sep 24, 2020
@hub33k
Copy link
Contributor Author

hub33k commented Sep 24, 2020

Rebased onto latest major.

@hub33k hub33k force-pushed the t/3582 branch 2 times, most recently from ce7ac74 to f376f37 Compare September 29, 2020 17:29
@hub33k
Copy link
Contributor Author

hub33k commented Oct 1, 2020

I have doubts about unit tests here. Do you have any suggestions what should be proper way to handle them? 🤔

@hub33k hub33k requested a review from f1ames October 1, 2020 08:37
@f1ames f1ames self-assigned this Oct 5, 2020
@f1ames f1ames removed the pr:stale 🗿 Old PR stale label. Currently "stale" label is used to mark both stale issues and PRs. label Oct 6, 2020
@f1ames
Copy link
Contributor

f1ames commented Oct 6, 2020

Rebased onto latest major.

plugins/autocomplete/plugin.js Outdated Show resolved Hide resolved
tests/plugins/autocomplete/viewposition.js Outdated Show resolved Hide resolved
tests/plugins/autocomplete/viewposition.js Outdated Show resolved Hide resolved
plugins/autocomplete/plugin.js Outdated Show resolved Hide resolved
@f1ames
Copy link
Contributor

f1ames commented Oct 6, 2020

Strange, it seems adding a comment to a previous review comment, posts pending review comments and commits the review 🤔 🤔 🤔  I'm still reviewing this PR, will post update when ready.

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.

The code

Generally looks good 👍

I see that we are not able to cover and test RTL left placement case due to lack of autocomplete RTL support so let's leave it for #4315. But let's leave manual tests in the current state too so we will be able to reuse them when working on it.

It seems that top placement case is not covered too. It is kind of an edge case but can happen with longer editor content:

Peek 2020-10-06 16-09

Manual tests

Very nice idea with manual tests and buttons which changes editors placement 👍 Only some wording suggestions here and there.

Thanks to these tests I have discovered that autocomplete/mentions doesn't support RTL in fact... as already mentioned above, extracted to #4315.

Unit tests

Strangely, some unit tests fails when run from dashboard (http://localhost:1030/#tests/is:unit,path:/tests/plugins/autocomplete) but passes when run separately. Probably something wrong with positioning/sizing stubbing 🤔 Tested on Chrome:

Also you have introduced rect.right which is passed to getViewPositon() so now all unit tests stubbing the rect should use it too.

As for approach to unit tests it looks fine. We need to stub things here since there is no other reasonable solution TBH. If you decide to switch to getViewPaneSize() it should be even easier to stub using sinon.

plugins/autocomplete/plugin.js Outdated Show resolved Hide resolved
plugins/autocomplete/plugin.js Outdated Show resolved Hide resolved
tests/plugins/autocomplete/manual/windowborder.md Outdated Show resolved Hide resolved
tests/plugins/autocomplete/manual/windowborder.md Outdated Show resolved Hide resolved
tests/plugins/autocomplete/manual/windowborder.md Outdated Show resolved Hide resolved
tests/plugins/autocomplete/manual/windowborder.md Outdated Show resolved Hide resolved
@Dumluregn
Copy link
Contributor

Rebased onto the latest major.

@Dumluregn Dumluregn requested a review from f1ames December 7, 2020 12:27
@Dumluregn Dumluregn removed their assignment Dec 7, 2020
@f1ames f1ames self-assigned this Dec 8, 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.

LGTM 👍

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.

Mention panel should consider window borders for positioning
3 participants