Fix for "find searches only part of the file" bug #6905
Conversation
|
|
|
bot please update playwright snapshots |
| @@ -6,7 +6,7 @@ | |||
| "command": "documentsearch:start", | |||
| "keys": ["Accel F"], | |||
| "selector": ".jp-mod-searchable", | |||
| "disabled": true | |||
| "disabled": false | |||
There was a problem hiding this comment.
Since these settings only include the keyboard shortcut override, maybe we could actually remove the whole file?
|
Thanks @ericsnekbytes for working on this! I left a small suggestion above to clean up unused settings files, but otherwise should be good to go. Maybe we can also mention this in the documentation as part of this PR? |
|
cc @yuvipanda for awareness since you had originally commented on this on the RetroLab issue: jupyterlab/retrolab#294 (comment) As explained in #6697, the motivation for reverting this change is the new virtual implementation of the notebook in JupyterLab (and in Notebook 7). Some cells might not be visible on the page until they are in the viewport. If we keep the shortcut disabled, then the browser search bar is still triggered on So with this PR it's mostly a matter of changing the default. Users who still want the browser search (even though it would not work on hidden elements) would have to disable the shortcut via the settings. |
docs/source/notebook.md
Outdated
| Your browser's `find` function will give unexpected results because it doesn't have | ||
| access to the full content of a document, but you can still use your browser find | ||
| function from the browser menu if you want, or you can disable the intelligent search | ||
| shortcut using the advanced settings editor. TODO: DETAILS (investigate settings bug?) |
There was a problem hiding this comment.
Should we track this TODO in an isssue?
There was a problem hiding this comment.
And if it's a more general issue about settings maybe we can track it on the JupyterLab repo directly: https://github.com/jupyterlab/jupyterlab
There was a problem hiding this comment.
@jtpio I've verified that the settings JSON schema that shows up in the advanced settings editor is different than your sample video. The issue is that I don't know the expected schema for the JSON object being edited, and thus how to explain how to disable the shortcut to a user.
There's also the matter of explaining to users what a JSON object is and what a key/value pair is, on top of where those go in the schema to properly disable the shortcut...this is more content that should be communicated in a docs entry. At this point I'm wondering if simply acknowledging the behaviors of intelligent-search and browser-search (which is already done in the docs in this PR) is enough for now...we can merge this in and worry about docs for disabling later. Thoughts?
There was a problem hiding this comment.
simply acknowledging the behaviors of intelligent-search and browser-search (which is already done in the docs in this PR) is enough for now...we can merge this in and worry about docs for disabling later.
Yes, and this is likely something to address in JupyterLab actually.
But it would be nice to not leave the TODO in the docs before merging.
There was a problem hiding this comment.
@jtpio That's done, I'm opening a new PR for Lab too and will link it here.
There was a problem hiding this comment.
Same docs updates for JupyterLab are at: jupyterlab/jupyterlab#14667
There was a problem hiding this comment.
Thank you for working on this @ericsnekbytes. This looks good to me. Please find a suggestion on capitalization for consistency below.
Co-authored-by: Andrii Ieroshenko <ieroshenkoa@gmail.com>
As suggested in #6697, this PR enables the Jupyter Document Search shortcut as a default behavior, so that users will get it by default instead of the browser find tool (Lab and Notebook 7 now only render visible-on-page content in the DOM, so the browser find is not going to work for file content outside the visible/viewport range).