Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 ctrl-f/g support for notebooks and text files #5795
Add ctrl-f/g support for notebooks and text files #5795
Changes from 15 commits
48a8fed
a1a84d2
891ae5d
142166c
d9408ea
01804c0
8aea812
7d3989e
4f746d0
d94d150
544ae5e
bf0ccbd
181bd04
4d978f1
667a4e9
744cefd
58a81f9
42f4d73
14a318e
525b1a8
272af34
ad1396f
d76f1a8
5f206c3
e4ed607
b1d95bb
7b3bef3
184074e
d75e5c5
d5ac6fb
fb7f998
53aea0f
046c830
3478507
9a0b5dc
38224e7
f052f5c
9e9eab5
6955cb0
0d048a5
52c75fb
de093b1
337a628
8bef9a2
ea774b3
31e0484
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a reason that
anchor
andhead
can use the function invocation way, butfrom
andto
need the bind? Seems like these four should be the same, or could use a comment explaining why they are different.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, CodeMirror's Range type takes
anchor
andhead
as values andfrom
andto
as functions. I'll have to double check my assumptions here about the values desired here. There's a bit of annoying translation needed in some places between the CodeMirror Range type and the CodeEditor IRange.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from
andto
are functions that basically pick off the min and max of the anchor/head. See https://github.com/codemirror/CodeMirror/blob/5b949b2dbe92a0153c2bd32b75341e7fe3cf6ce0/src/model/selection.js#L51-L59There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should just be using the CodeMirror Range object directly here? Can we import and use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I was reading it wrong. I think you are eventually going to call this function: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/89592f7ec313174363136fe91749cedff272a70e/types/codemirror/index.d.ts#L294
That function needs a
{from: CodeMirror.Position, to: CodeMirror.Position}
range, not aCodeMirror.Range
object. So I think we perhaps don't need this private function after all?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or at least, if we do want this private function, it is not returning a
CodeMirror.Range
, just a{from: position, to: position}
object.I can understand the confusion. The CodeMirror docs say it takes a range, but looking at the codemirror source, it's clear it wasn't talking about a
CodeMirror.Range
object, but rather a simple object withfrom
andto
positions (not functions).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a patch like this should make this bit of code work:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aschlaep - I think this should work for now, and we can delete the extra argument to the scrollIntoView if we submit a change to the codemirror typings to make the margin argument optional in https://github.com/DefinitelyTyped/DefinitelyTyped/blob/ee0aad13bc3dd0f0176ca6794aacc0227d88c0fe/types/codemirror/index.d.ts#L286 and https://github.com/DefinitelyTyped/DefinitelyTyped/blob/ee0aad13bc3dd0f0176ca6794aacc0227d88c0fe/types/codemirror/index.d.ts#L294