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

T/37: The mention UI should prefer position below the caret. #52

Merged
merged 9 commits into from
Apr 24, 2019
Merged

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Apr 10, 2019

Suggested merge commit message (convention)

Fix: The mention UI should prefer position below the caret. Closes ckeditor/ckeditor5#4643.


Additional information

  • The only change that was preserved is the order of position returned by getBalloonPositionData()

  • I've added some padding text to the manual test to overcome short body problem.

  • It does look OK now in the docs:

Selection_298

@jodator jodator requested a review from oleq April 10, 2019 12:47
@mlewand mlewand changed the title T/37: The mention ui should prefer position below the caret. T/37: The mention UI should prefer position below the caret. Apr 17, 2019
jodator and others added 2 commits April 18, 2019 09:00
@oleq
Copy link
Member

oleq commented Apr 19, 2019

TBH, this looks like a bug to me:

image

I agree that we should prefer the positions below the caret, but if that means the panel will stick out of the editor even though there is enough space to accommodate it inside it, it's a bad practice IMO.

We don't know what's below the editor, it could be some form or other application UI that matters to the user. Or the editor could be in a modal, and since we're using a floating panel stored directly in <body>, it would position correctly with respect to the editor, but it would stick out of the modal. And there's no way for developers to change that behavior.

The rule of thumb should be that the editor keeps its elements within the editable whenever possible because this is the only space we can safely use. The rest is a gamble for us because it depends on the integration.

So I'm for changing the order of prefered panel positions but restoring the limiter back to editable to stay on the safe side. Even though, in some cases, it will look like this

image

it is still better IMO than the other way around because it's not CKEditor territory we'd use.

cc @Reinmar

@mlewand mlewand self-requested a review April 23, 2019 07:55
@mlewand mlewand self-assigned this Apr 23, 2019
@mlewand mlewand requested review from oleq and removed request for oleq and mlewand April 23, 2019 07:56
@mlewand mlewand removed their assignment Apr 23, 2019
@mlewand
Copy link
Contributor

mlewand commented Apr 23, 2019

Restoring @oleq as reviewier as I didn't noticed that changes requested in a previous comment are not applied yet.

@jodator
Copy link
Contributor Author

jodator commented Apr 23, 2019

OK guys, we need to do something with this one: either we incorporate this PR (in line with @Reinmar suggestions) or simply close it as previously the limiter was set to the editable area as @oleq proposed.

@mlewand
Copy link
Contributor

mlewand commented Apr 23, 2019

Just to throw 2 cents here, I'd second to that it makes more sense to put dropdown inside of editor region whenever possible.

@Reinmar
Copy link
Member

Reinmar commented Apr 23, 2019

@oleq's arguments make a lot of sense. I still have some doubts, but I retract my vote (including my ticket).

@jodator
Copy link
Contributor Author

jodator commented Apr 23, 2019

@oleq's arguments make a lot of sense. I still have some doubts, but I retract my vote (including my ticket).

So I'll cleanup this PR so only "prefer south" position will be preserved.

@jodator
Copy link
Contributor Author

jodator commented Apr 23, 2019

@oleq OK I've revamped the PR and made sure that mention balloon would prerfer south positions.

@oleq oleq merged commit 63cd822 into master Apr 24, 2019
@oleq oleq deleted the t/37 branch April 24, 2019 11:21
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The autocompletion panel should prefer the south positon
4 participants