Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

implement find in page #111

Merged
merged 2 commits into from
Dec 28, 2015
Merged

implement find in page #111

merged 2 commits into from
Dec 28, 2015

Conversation

diracdeltas
Copy link
Member

known issues:

  • ESC is handled by a local shortcut, so this uses SHIFT+ESC to close the findbar
  • Showing number of matches doesn't work yet
  • Case-sensitivity state isn't preserved when the findbar is re-displayed

Auditors: @bbondy

@diracdeltas
Copy link
Member Author

Also, this uses electron-prebuilt 0.36.2

@bbondy
Copy link
Member

bbondy commented Dec 28, 2015

In addition to the things you mentioned, this is also needed:

  • Incremental search, as you type it selects without having to press enter
  • Placeholder text isn’t showing up on the find bar
  • The find bar in brave/browser is probably closer to the look we want since it only takes up the top right space.

screenshot 2015-12-28 13 24 38

@bbondy
Copy link
Member

bbondy commented Dec 28, 2015

Looking good so far, thanks for taking this.

@diracdeltas
Copy link
Member Author

incremental search should be working as of latest commit

@bbondy
Copy link
Member

bbondy commented Dec 28, 2015

Oops I think I had an older version of your branch pulled with the white bar going all the way across too. The newer update looks great. Feel free to merge and then do the rest as follow ups.

@diracdeltas
Copy link
Member Author

thanks, merged

@diracdeltas
Copy link
Member Author

oops, apparently pull req with failing CI can't be merged from a phone. i'll do it later

diracdeltas added a commit that referenced this pull request Dec 28, 2015
@diracdeltas diracdeltas merged commit 06452b6 into master Dec 28, 2015
@diracdeltas diracdeltas deleted the feature/find-in-page branch January 14, 2016 21:01
diracdeltas added a commit that referenced this pull request Jun 26, 2017
This reverts commit 91ef559.

it also re-fixes #9308 after the revert and fixes handling of skipSync (#111)

Test Plan:
Test plan in brave/sync#111
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants