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

Add option to avoid injection menu in document body #76

Closed
wants to merge 1 commit into from

Conversation

FMRb
Copy link

@FMRb FMRb commented Feb 10, 2015

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 79.53% when pulling efc0d06 on FalconSocial:mentio-menu-not-body into 4cefa13 on jeff-collins:master.

@FMRb FMRb closed this Feb 10, 2015
@FMRb
Copy link
Author

FMRb commented Feb 10, 2015

I have found an issue on our website regarding the mentioMenu directive. It assumes that the element has to be injected on the body, however, this injection provokes a misbehaviour of the mentioMenu with the scrolling of the page, since the scroll event is not focused on the body but in another container.
This PR tries to modify the injection and recalculate the position base on the container where the mentioMenu is invoked.
I have closed this PR, since this is a hack to make it work on our website but it doesn't cover all the cases.

@jeff-collins
Copy link
Owner

thanks for taking a look at this, happy to take a look. can you give me a small snippet of the kind of page that is causing this behavior with ment.io?

@FMRb
Copy link
Author

FMRb commented Feb 11, 2015

Hi Jeff,
I have created a Plunker with a small example of what happens in our website.
http://plnkr.co/edit/gcLd964czcETfpQbJryy?p=preview

Opening this snippet and using the scrolling on the area where the contenteditable container is located will make the mentioMenu move respect to the body instead of the contenteditable container itself. My fix tries to avoid this issue injecting the mentionMenu on the container where the contenteditable is positioned.

I hope this helps to clarify the issue.

@jeff-collins
Copy link
Owner

perfect, thanks. i think one possible fix might be to retrigger the menu placement - which is what happens in other cases. i'll take a look.

@becomingbabyman
Copy link

+1

@kkulikowski
Copy link

Hello. @jeff-collins, is there any chance to merge the solution? I wounder should i wait or just implement it locally.
Regards!

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.

None yet

5 participants