Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Use proper method to map a view to a DOM element in getBalloonPositio… #88

Merged
merged 1 commit into from
Jun 27, 2019

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Jun 24, 2019

Use proper method to map a view to a DOM element in getBalloonPositionData().

Suggested merge commit message (convention)

Fix: Use proper method to map a view to a DOM element when getting balloon position data. Closes ckeditor/ckeditor5#4608.


Additional information

  • I don't see a need for adding tests as this is more a typo then a bug but feel free to R- this :)

@jodator jodator requested a review from Reinmar June 24, 2019 15:17
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 71671ef on t/87 into 79ec161 on master.

@jodator jodator requested review from oleq and removed request for Reinmar June 25, 2019 13:12
@@ -243,7 +243,7 @@ function getBalloonPositionData( editor, relatedElement ) {
const defaultPositions = BalloonPanelView.defaultPositions;

return {
target: editingView.domConverter.viewToDom( relatedElement ),
target: editingView.domConverter.mapViewToDom( relatedElement ),
Copy link
Member

Choose a reason for hiding this comment

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

We need a test for this change.

Copy link
Contributor Author

@jodator jodator Jun 27, 2019

Choose a reason for hiding this comment

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

I'm trying to write a test for this. And the only case when this fails if the getRelatedElement() will return an object with API similar to view element (ie Model's Element) and the viewToDom will try to create a DOM node and somehow the domDocument is null. I'm lost how to write a test for this.

Also, this issue is really about the wrong method use with wrong parameters. Also changing viewToDom to proper mapViewToDom does not fix the issue with wrong API use.

@oleq oleq merged commit 160333a into master Jun 27, 2019
@oleq oleq deleted the t/87 branch June 27, 2019 10:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants