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

Desktop: Toggle Editor rather than setting split mode on search #3561

Merged
merged 3 commits into from
Aug 1, 2020

Conversation

CalebJohn
Copy link
Collaborator

reference

Before I was manually setting split view which was particularly annoying for users who had disabled the split view. This is how I should have implemented it originally.
Before merging this I'd like to get some community feedback about the current behaviour.

@laurent22
Copy link
Owner

laurent22 commented Jul 26, 2020

I think the issue is that we're trying to sync two views that can't be synced since what's highlighted in the editor view will be different than what's in the viewer. The currently highlighted term will also be different in most cases.

The only way to resolve this is to have a search bar per view, like it's done in VSCode for example:

image

Having two search bars might quite a bit of work so it would be good to find a simpler solution for now. I'm thinking maybe one of these options:

  • We simply disable searching in the viewer, so we make it clear that it's not implemented yet.
  • Or we keep it on, but we don't automatically toggle the split mode. Instead we display a message to the right of the search bar that says "Note: As of now, searching is only enabled in the editor view"

What do you think?

@CalebJohn
Copy link
Collaborator Author

I think option 2 is the better way to go. Some users might like the keyword highlighting so there is no sense reducing functionality. I can remove the code that causes the splitting. But I'm not sure how to go about adding the message though. Should we be passing another function to the props of codemirror?

@CalebJohn
Copy link
Collaborator Author

I ended up going with option 2. Along with the message I also disabled the jump buttons and the jump counter. Toggling the editor brings the buttons back and the currently selected item stays stable across toggles.

You'll notice in the code I added more SEARCHHACK comments, thats because I currently use the match counter to signal to the search bar that the codemirror editor is in use. Once we remove ace editor this won't be needed anymore and the code can be cleaned up. I feel like this is a reasonable way to get that behaviour without doing much refactoring that would just be un-done.

@laurent22
Copy link
Owner

Looks good, thanks for the update @CalebJohn!

@laurent22 laurent22 changed the base branch from master to dev August 1, 2020 18:07
@laurent22 laurent22 merged commit bab29cd into laurent22:dev Aug 1, 2020
@CalebJohn CalebJohn deleted the split branch August 1, 2020 18:40
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.

2 participants