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

Add command to open find with replace #7725

Merged
merged 5 commits into from Feb 15, 2020
Merged

Conversation

mlucool
Copy link
Contributor

@mlucool mlucool commented Jan 2, 2020

Adds a Accel H command by default which opens the find box with replace expanded. This mirrors how most editors and Microsoft products work.

Code changes

Refactored the find command to add a find + replace option.

User-facing changes

New default hotkey added.

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Jan 2, 2020

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jan 2, 2020

A quick survey:

  • VS Code: Replace is Alt Accel F (Replace in Files is Shift Accel H)
  • MS Word: Replace is Shift Accel H

Also:

  • Sublime Text: Replace is Alt Accel F
  • PyCharm: Replace is Control R
  • Atom: Replace in Buffer is Alt Accel F
  • macOS text edit: Replace is Alt Accel F

On a mac, at least, it looks like the natural shortcut would be Alt Accel F. Thoughts?

@jasongrout jasongrout added this to the Future milestone Jan 2, 2020
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jan 2, 2020

Refactored the find command to add a find + replace option.

But first, thanks for doing this!

@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Jan 2, 2020

Interesting. I am using Windows 10 here is what I see:

Wordpad: Ctrl-H
Office Professional Plus 2016: Ctrl-H
Webstorm/PyCharm: Ctrl-R
Google Docs: Ctrl-H
Visual Studio Code: Ctrl-H

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jan 2, 2020

For Google Docs on macOS/Firefox, I see Accel Shift H brings up Find (and replace happens to be possible too from the dialog).

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jan 2, 2020

In 2.0, we'll have the ability to have platform-specific shortcuts. However, there is a philosophical decision - should we be consistent across platforms (like pycharm), or should we be consistent with the platform?

@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Jan 3, 2020

I lean slightly towards platform specific hotkeys when there is a clear standard. This gives users a more "native" experience and matches what both google and msft have done. It lets us avoid using hotkeys that have meaning on one platform (e.g. Accel H to hide on mac).

Where is platform decided? I run my lab on linux but my browser is windows.

@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Jan 11, 2020

I see this was moved to milestone future. Will this not be able to make the 2.0 cut?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jan 11, 2020

It can still make it into 2.0 since it's not backwards incompatible. I think the biggest blocker at this point might be deciding the default keyboard shortcut. A way around that discussion is to take off the default keyboard shortcut for now (and users would be able to assign a custom keyboard shortcut).

Hotkeys are a complex issue with UX implications of consistency for the app vs. the platform a user is on. Removing default hotkeys to delay this decision.
@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Jan 11, 2020

@jasongrout the hotkey has been removed. Please let me know of any other changes you'd like

@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Jan 23, 2020

Ping @jasongrout as I am hoping to for this to make it into 2.0!

@saulshanabrook saulshanabrook self-requested a review Feb 12, 2020
Copy link
Member

@saulshanabrook saulshanabrook left a comment

I tried this out locally and it works as expected.

I find the diff a bit confusing on github, but looking at it locally I can see this doesn't change much. It just

  1. Factors out current command isEnabled and execute into new functions.
  2. Adds a new command that uses those functions to also open the find and replace after.

I renamed those two functions to try to be more clear about their intent.

One issue is that currently if you already have the find box open, and run this command, then it will not open the replace box.

However to fix this would require a deeper refactor in how this provider is set up. It passes the whole _displayState to the React component which then won't re-render because it just uses it for the default props, I believe and the object doesn't change.

IMHO It's a bit of a mess currently, because the Search provider thing tries to not be aware of react, so you have these two different concepts of state management fighting over each other.

@mlucool
Copy link
Contributor Author

@mlucool mlucool commented Feb 13, 2020

IMHO It's a bit of a mess currently, because the Search provider thing tries to not be aware of react, so you have these two different concepts of state management fighting over each other.

When I first worked on the improved find functionality, I also found this state in two places somewhat hard to grasp.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Feb 15, 2020

Thanks!

@blink1073 blink1073 merged commit 2763b5b into jupyterlab:master Feb 15, 2020
8 of 10 checks passed
@jasongrout jasongrout removed this from the Future milestone Feb 24, 2020
@jasongrout jasongrout added this to the 2.0 milestone Feb 24, 2020
@lock lock bot added the status:resolved-locked label Mar 27, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:documentsearch status:resolved-locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants