Added possibility to attach balloon panel to selection instead of el… #1526
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
engineering-this
force-pushed
the
t/1498
branch
from
4fa6edd to
306b2e0
Compare
engineering-this
changed the title
engineering-this
force-pushed
the
t/1498
branch
6 times, most recently
from
3f15c75 to
4fae2a6
Compare
engineering-this
force-pushed
the
t/1498
branch
7 times, most recently
from
0a0592c to
dfa498b
Compare
|
Please rebase to latest |
mlewand
added
the
review:easy
|
Rebased onto latest |
Comandeer
requested changes
Jul 19, 2018
There was a problem hiding this comment.
As it's stated in issue:
Instead we'd like to attach it to the center of a selection.
However, as manual test shows, the panel is attached to the end of the selection.
There is also issue with selections with multiple ranges, e.g. made by tableselection plugin:
CHANGES.md
Outdated
| @@ -8,6 +8,7 @@ New Features: | |||
| * [#2072](https://github.com/ckeditor/ckeditor-dev/issues/2072): [UI Button](https://ckeditor.com/cke4/addon/button) plugin supports custom `aria-haspopup` property values. [Menu Button](https://ckeditor.com/cke4/addon/menubutton) `aria-haspopup` value is now `menu`, [Panel Button](https://ckeditor.com/cke4/addon/panelbutton) and [Rich Combo](https://ckeditor.com/cke4/addon/richcombo) `aria-haspopup` value is now `listbox`. | |||
| * [#2154](https://github.com/ckeditor/ckeditor-dev/issues/2154): [Link](https://ckeditor.com/cke4/addon/link) plugin now supports telephone number links. | |||
| * [#1815](https://github.com/ckeditor/ckeditor-dev/issues/1815): [Autolink](https://ckeditor.com/cke4/addon/autolink) plugin supports typing link completion. | |||
| * [#1526](https://github.com/ckeditor/ckeditor-dev/pull/1526): Added possibility to attach balloon panel to selection instead of element. | |||
There was a problem hiding this comment.
Changelog entry should link to the issue, not to the PR. Additionally it'd be nice to link the plugin.
plugins/balloonpanel/plugin.js
Outdated
| @@ -319,7 +319,8 @@ | |||
| * touches that element. Once the panel is attached it gains focus. | |||
| * | |||
| * @method attach | |||
| * @param {CKEDITOR.dom.element} element The element to which the panel is attached. | |||
| * @param {CKEDITOR.dom.element/CKEDITOR.dom.selection} element The element to which the panel is attached. | |||
| * **Since 4.10.0** instead of an element it is possible to pass a selection {@link CKEDITOR.dom.selection}. | |||
plugins/balloonpanel/plugin.js
Outdated
| if ( element instanceof CKEDITOR.dom.selection ) { | ||
| var ranges = element.getRanges(), | ||
| rectList = ranges[ 0 ].getClientRects( true ), | ||
| rect = rectList[ rectList.length - 1 ]; |
| @@ -0,0 +1,70 @@ | |||
| <textarea id="editor_classic" cols="10" rows="10"> | |||
There was a problem hiding this comment.
I don't understand why this test is called textpanel. IMO the name should contain word "selection" (e.g. selectionpositioning).
| @@ -0,0 +1,9 @@ | |||
| @bender-ui: collapsed | |||
| @bender-tags: 4.10.0, feature, balloonpanel, 1176 | |||
| return selection; | ||
| }, | ||
|
|
||
| assertBalloonPanelPosition: function( rect, expectedX, expectedY ) { |
There was a problem hiding this comment.
This function is not only asserting, but creating the whole test case for balloon panel.
Additionally I don't see clearly which tests are for elements and which are for selection.
engineering-this
force-pushed
the
t/1176
branch
5 times, most recently
from
cec38a7 to
0165273
Compare
…o horizontal center
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note:
This branch is based on branch from PR #1499 ( t/1498 ).What is the purpose of this pull request?
New feature
Does your PR contain necessary tests?
All patches which 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
What changes did you make?
I've added option to pass a selection as first argument of
CKEDITOR.ui.balloonPanel.attach(). In that case balloonPanel will attach itself to selection instead of element.Closes #1176