Add replace

[SundeepChand]

Fixes #1519 & Fixes #1588

I have verified that this pull request:

• has no linting errors (npm run lint)
• is from a uniquely-named feature branch and has been rebased on top of the latest master. (If I was asked to make more changes, I have made sure to rebase onto master then too)
• is descriptively named and links to an issue number, i.e. Fixes #123

Please review this and tell me if it has something missing. Since I'm no good in Spanish, please make sure that the Spanish translations are accurate.

[catarak]

Thanks for opening this PR! It seems like you added the functionality, but the "Find" Popup looks exactly the same and it's unclear to me how a user would input a replace term.

[SundeepChand]

Okay, so do you mean, we should add replace term input to Find Popup as well? So instead of having a separate Replace Popup, we can extend the Find Popup to support Replace as well.

[catarak]

Okay, so do you mean, we should add replace term input to Find Popup as well? So instead of having a separate Replace Popup, we can extend the Find Popup to support Replace as well.

Oh, I didn't realize there was a separate replace popup! Yes, I think it would make sense to combine them as that's pretty typical of find/replace designs. Let me know if you'd like some ideas for how to combine these designs!

[SundeepChand]

Sure. I would like to work on this further.

[SundeepChand]

@catarak I have modified the replace dialog this way. May I have your opinion regarding this. And yeah its collapsible.

[catarak]

@SundeepChand I think that maybe you forgot to commit codemirror-search.js? Because when I bring up the find popup, it still looks like this:

And when I bring up replace popup, it looks like this:

[SundeepChand]

@catarak Actually I've done only the layout part so far. I'm working on adding the functionality now. Once I'm done with that, I will push the code.

[SundeepChand]

@catarak Please check if the find & replace popup works fine. Still there are a couple of bugs in this commit. When replace is done with regex set to true, it exits only after one replacement. There are certain other incomplete things as well, which I'm still trying to figure out. As well as I think instead of having separate Replace & Replace All options in Edit drop-down, we can have a single Replace option.

[SundeepChand]

@catarak I think its mostly done. Please check this out and let me know if anything is not working.

[catarak]

This is so awesome! It seems like the functionality is all there and my suggestions are UI/UX related:

• I think the location of the button to expand/contract the replace section isn't great. I think the placement is confusing
  because it implies it's related to changing the functionality of "Find".
  
  One solution that some tools use is have a button with a caret that expands/contracts the "Replace" mode area:
  

• When in "Replace" mode (and not "Replace All" mode), you can't tell which text is selected that you are replacing, whereas in search mode, the text is highlighted

• I don't like how the popup shrinks when you're in "Replace" mode, and I think you did this so that it doesn't block the sketch text. Maybe there's a way to utilize more horizontal space? Similar to the CodeSandbox search above?

Thanks for doing so much work on this!

[SundeepChand]

Sorry for the late response, as I was a little busy with my university work. Surely, I will update the branch, once I make it better.

[SundeepChand]

I have fixed the layout of the find popup, but so far I couldn't manage to fix the second issue. I also want to add the replace buttons icons, but couldn't find any.

[SundeepChand]

@catarak The layout of the popup is done now.

[SundeepChand]

@catarak Please check this out. I have fixed all the issues including the text-highlighting in replace mode. Fixing this was a little bit tricky for me, but I'm glad that its done now, and I got to learn more about the codemirror functions.

[catarak]

This is looking so awesome! I think it's really close. I just added a few minor styling changes. Most significantly, I replaced the "replace" and "replace all" icons with text. Feel free to revert those changes, but for me, I think the "replace" and "replace all" icons are pretty cryptic and I always have to hover over them to figure out which is which. And there's room for the text, so why not put the text?

Do you think it would be possible for the replace mode to not switch to the other view? The UX I imagine would be:

1. Click "Replace"
2. Every subsequent time the user clicks "Replace", it replaces the next occurrence (which is highlighted).
3. When the user clicks on the "^" and "v" arrows, it will jump the selected occurrence, and then clicking "Replace" would change that occurrence.

Does that make sense? Let me know if you need clarification.

[SundeepChand]

Yeah I got it. Its going work the way the popup works in codesandbox / VSCode. Will be doing it soon.

[catarak]

Yeah I got it. Its going work the way the popup works in codesandbox / VSCode. Will be doing it soon.

Thanks for taking my feedback and working so hard on this!

[SundeepChand]

@catarak Please check this out & let me know if anything is missing. And yes, please verify the Spanish translations as well.

[catarak]

Thanks for your changes! I'm not sure how to integrate the Spanish translations because across the rest of the application, they're passed to React components via props, and this is obviously not rendered with React. I wonder if there is a way to render this component with React? Anyway, that's a separate issue (#1588).

[SundeepChand]

I think it would be better if we deal with that in a separate pull request. But I was talking about the translations which I have written in 'translations/locales/es-419/translations.json' file.

[catarak]

@oruburos would you mind reviewing the following Spanish translations made as part of this PR:
"Replace": "Reemplazar"
"Replace All": (this one needs to be translated)
"ReplaceTextMatch": "Reemplazar coincidencia de texto"

Thank you!!

[oruburos]

Hi @catarak

"Replace": "Reemplazar" ok
"Replace All": "Reemplazar todas las coincidencias" here is my suggestion
"ReplaceTextMatch": "Reemplazar coincidencia de texto" ok

[catarak]

Sorry for the delay merging this in! I'm not super happy with the Find/Replace UX, which is basically the default way the Codemirror find/replace works. I've been doing a little research on how it to fix it though!

[SundeepChand]

Sorry for the delay merging this in! I'm not super happy with the Find/Replace UX, which is basically the default way the Codemirror find/replace works. I've been doing a little research on how it to fix it though!

That's totally fine. Please let me know about the issues, if I can help in any way.

[catarak]

@SundeepChand I added some changes to update the CodeMIrror search to be closest to the latest CodeMirror release, and also to match the UX of VSC. It would be awesome if you could test/review and let me know what you think!

[SundeepChand]

Hello @catarak! Thanks for working on this. I tested it & yeah the UX feels much nicer now. But I found some bugs in the "Case Insensitive" & "Whole word" buttons. When toggled, they don't show any effect. I've fixed the "Case Insensitive" one by changing some code-mirror defaults. And I need a little time to fix the other one.

[SundeepChand]

Hi @catarak ! I think its ready to be merged. Please review it. I've changed the parseQuery() method to the older version of code-mirror to fix the bugs in the functionality of the 3 toggle-buttons.

[catarak]

@SundeepChand thanks for your fixes! Sorry for breaking stuff that you had already worked on. I just pushed some changes to fix the icon colors in each theme. Would you mind testing?

[SundeepChand]

@catarak Please checkout the changes. I've changed the highlight-color in high-contrast mode as it was barely visible. Feel free to revert that in case if its not ok. I would like to improve the keyboard accessibility of the dialog, but since I'm a little busy for a couple of days, I won't be able to work on this right now. Maybe we can do that in a separate issue.

[catarak]

Thanks for your updates! I changed the colors a bit more. In order to keep the color contrast with WCAG AAA guidelines, I decided to add an outline for the selected text for the high contrast mode.

I think it makes sense to update the keyboard accessibility in another issue! This PR has gotten pretty big so it's probably best to merge it in :)

[catarak]

I think this is finally ready 🎉