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

Address #3695 using data-codemirror-linenumber and CSS content #3696

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

Conversation

colelawrence
Copy link
Contributor

This addresses the open issue: #3695

The code presented is linted, and all tests were passed.

You may view a demo with changes off my gh-pages branch

Please provide feedback on recommended changes or styles.

@danth
Copy link
Contributor

danth commented Sep 18, 2017

:lgtm_strong:

Everything looks good to me. All coding styles are the same as surrounding code, there are no obvious errors.


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@adrianheine
Copy link
Contributor

This would have to be rebased since I split lib/codemirror.js a year ago.

@marijnh
Copy link
Member

marijnh commented Oct 20, 2017

I think this is too invasive. The plan is to eventually, in a breaking version, move all line gutter content to a separate container so that they don't muddle up the code's DOM.

@colelawrence
Copy link
Contributor Author

@marijnh I agree that a change like this should probably be considered for a major release as it could break a plugin that depends on reading the DOM structure for line numbers.

I wonder if just using CSS user-select: none could also address this...

@marijnh
Copy link
Member

marijnh commented Oct 20, 2017

I wonder if just using CSS user-select: none could also address this...

Unfortunately, no. That'll hide the selection markers, but still copy the text on at least some browsers.

@colelawrence
Copy link
Contributor Author

colelawrence commented Oct 20, 2017 via email

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

4 participants