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

fix for 'clicking on overlay does nothing' #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

boogie666
Copy link

I'm assuming that this is what the readme text means when it says
"Clicking on overlay does nothing".

I assume that clicking on a marker (i.e. a colored paren) should move the cursor there.
This commit fix this so that it does move the cursor there.

My only concern is that there are quite a few listeners added in to the mix.
Basically one for each marker.

Don't merge this in yet please. Just let me know if this is the correct behaviour.

@johngeorgewright
Copy link
Owner

Thanks for this! I'm wondering, however, about if there's a more performant way to do this. I'm unsure about the implications of adding an event listener to every marker (which is an unknown amount). Perhaps there's a way to catch the event during the event bubbling process.

@boogie666
Copy link
Author

Since atom is just a browser, basically adding a listener to the parent element will work as you describe. I'll fix this in my PR on monday. I'm camping atm so no laptop :P.

@boogie666
Copy link
Author

found this https://atom.io/packages/swackets it's basically same exact thing as what you're trying to do here. but with a totally different approch to setting the colors.

basically it tries to find the underlying span element for each type of brace and adds the correct color class to it, instead of adding a marker to the editor.

this direct dom manip approch seems much better and more lightweight and it also renders this pull useless since there are no more markers to handle.
and it also solves the other issue you have with the markers not moveing correcly.

I've create a new branch on my fork called 'manip-the-dom' where i've adapted your code to just add classes to the spans directly and it works quite well :)

!!! Note that it's just a basic (rather stupid) POC :P . currently it only works on js syntax because of how i select the spans and does not handle code folds at all !!!.

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.

2 participants