Fix find first result centering #4339
Conversation
JeffryBooher
commented
Jun 25, 2013
JeffryBooher
commented
- Adds Editor.isLineVisible() API
- Fixes issue First Find result not vertically centered in this case #3400
ghost
assigned 
|
@dangoor this should be good to go. |
| */ | ||
| Editor.prototype.isLineVisible = function (line) { | ||
| var $scrollerElement = $(this.getScrollerElement()), | ||
| coords = this._codeMirror.charCoords({line: line, ch: 0}), |
There was a problem hiding this comment.
Do we need to specify mode here "local" or "page". Seems like there might be an issue if the html menu or modal bar (find, quick open, etc.) is visible.
There was a problem hiding this comment.
The mode param is optional and so far this hasn't been an issue (with find anyway). I'm not sure which one of these you would specify if either those cases were true. Once find has determined if the line is visible or not it will scroll the line to the center of the display if it's covered by the modal bar at the top anyway so the modal bar hasn't been a problem with this -- i tried various cases when i wrote it and it just seemed to work as desired. Not sure if you can get into this state with a menu -- those should dismiss when a command is executed.
|
See screenshots. I disabled the shell menus by commenting lines 272-274 in brackets.js to move the editorholder position downwards.
|
|
It looks like we'll want to take this in sprint 28 after the bug Jason noticed has been squashed. (What Jeff said is true: this shouldn't affect the find feature. the trouble I see is that |
|
@jasonsanjose @dangoor try it now. I'm using scroll position and page relative result checking which so far appears to be bullet-proof |
|
scratch that... "Local" coordinates not "Page"... I had tried "Page" earlier without success. "Local" is what we want... |
|
@jasonsanjose @dangoor I may have dropped the ball on this. I should have let you all know that it is ready to merge now. |
| @@ -893,6 +893,21 @@ define(function (require, exports, module) { | |||
| }; | |||
|
|
|||
| /** | |||
| * Deterines if line is fully visible. | |||
| * @param {number} zero-based index of the line to test | |||
| * @returns {boolean} true if it's visible, false if not | |||
There was a problem hiding this comment.
typo @return. Also, change comment to true if the line is fully visible, false otherwise
There was a problem hiding this comment.
Nevermind, I fix it myself.
|
Looks good. Merging. |
…based Fix find first result centering
