Skip to content
This repository has been archived by the owner on Apr 22, 2018. It is now read-only.

Fixes #10 #17

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fixes #10 #17

wants to merge 4 commits into from

Conversation

cossssmin
Copy link

@cossssmin cossssmin commented Oct 24, 2016

Removes the need for a menu container that covers the page. This way, we can catch the clicked target and close the menu as needed.

Removes the need for a menu container that covers the page. This way, we can can catch the clicked target and close the menu as needed.
If menu is triggered on a page higher than the viewport, setting overflow: hidden on the body will make the content 'jump' to the right on operating systems that make the scrollbar take up space in the browser (i.e. Windows).
Following up on the previous commit, this is no longer necessary.
There's actually no need to check for clicks inside the container or items. As long as the context menu is found in the DOM, close() will simply remove it.
After the removal of the backdrop div, clicking two different basicContext-enabled elements would show the same menu on the second click. This fixes that behavior by always removing any menu before creating one.
@electerious
Copy link
Owner

Looks good and works very well. Thanks! We just need to get around the mentioned onclick problem.

@cossssmin
Copy link
Author

cossssmin commented Oct 25, 2016

We just need to get around the mentioned onclick problem.

Not sure which one that is, is it not working as expected?
If you're referring to this:

the user may try to right-click on a different element

That's already covered in the last commit (3376994). Clicking a different basicContext-enabled element while the menu is already open on one, produces the expected behavior: currently opened menu is removed, and the new one is opened on the new element that was clicked.

Or, maybe I'm missing something?

@electerious
Copy link
Owner

Not sure which one that is, is it not working as expected?

.onclick overwrites the original click-event of the parent element. We don't know the parent element. It could be anything. e.g. an element with an existing click-event and .onclick would overwrite it. addEventListener allows you to attach multiple functions for the same event without overwriting the existing ones. However, depending on how nested the button is that triggers the context and how the site uses event.stopPropagation(), it might fail to close in some situations. I guess this was the reason for me to use the transparent div. It has its downsides, but also eliminates a lot of edge cases.

The demos all work fine, because this case isn't covered.

I also see a problem with not blocking scrolling (previously done via overflow: hidden). This isn't covered in the demos, because you can't scroll the site in one of them. The context will either scroll with the site or stay fixed on its position, which will look kind of crazy for scrollable context menus.

@cossssmin
Copy link
Author

Thanks for the explanation.

For the onclick, I don't really know what we could do.

For the overflow/scroll, could we have it enabled just in the case of scrollable context menus? Looking at the default browser context menu in Windows, indeed you can't scroll when it's opened; but then again, it also doesn't remove the scrollbar.
So maybe setting overflow: hidden only for scrollable context menus, could be a compromise?

@cossssmin cossssmin changed the title Fixes https://github.com/electerious/basicContext/issues/10 Fixes @electerious/basicContext#10 Apr 27, 2017
@cossssmin cossssmin changed the title Fixes @electerious/basicContext#10 Fixes electerious/basicContext#10 Apr 27, 2017
@cossssmin cossssmin changed the title Fixes electerious/basicContext#10 Fixes #10 Apr 27, 2017
@rangitha
Copy link

rangitha commented Jul 5, 2017

@hellocosmin What's the status of this PR?

Is preventing default behaviour on right-click of basicContextContainer an intermediate solution for this problem?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants