ChromeDevTools / debugger-protocol-viewer

DevTools Protocol API docs—its domains, methods, and events
https://chromedevtools.github.io/devtools-protocol/
Other
862 stars 172 forks source link

Keyword search feature #34

Closed ericguzman closed 9 years ago

ericguzman commented 9 years ago

demo: https://ericguzman.github.io/debugger-protocol-viewer/

Summary of search feature:

image

ericguzman commented 9 years ago

@kdzwinel, @paulirish Thanks again for your suggestions. I think I've addressed them with this implementation, as well as added many UI improvements. Take a look and feel free to provide feedback: http://ericguzman.github.io/debugger-protocol-viewer/

It's not perfect but it should be production-worthy (anyone have a good icon for the category of "Method"?). Please let me know if you spot any blockers.

paulirish commented 9 years ago

Whoa. This is nice stuff. I like the UI!

Outside of that, I'm pretty happy. @kdzwinel I'll let you have a crack at this PR before green-buttoning it.

kdzwinel commented 9 years ago

Looks amazing, I love the icons in the search results :heart: Also, huge props for looking into the performance issues!

Code review of 38k of lines is challenging, especially because Github won't show me the full diff, but I caught these:

  1. I had a quick look at searchindex.json: screen shot 2015-09-29 at 00 43 44

    TBH I don't see why we have both href and domainHref there. It probably gzips well, so maybe it's a non-issue, but if it's simple to remove domainHref or reduce href just to a hash part, it'd be great.

  2. I observed that whenever I start searching new font file is loaded by the paper-ripple element. It looks like a polymer quirk, but maybe you'll know how to fix that?
  3. Since searchindex.json has to be rebuilt after each protocol.json lets make sure to mention that in the readme: https://github.com/ChromeDevTools/debugger-protocol-viewer#building
paulirish commented 9 years ago

@ericguzman looks like your commit is to address our comments. should we take another look?

ericguzman commented 9 years ago

Had a couple more cleanup items + merged upstream changes. All done now, so please take a look.

On Mon, Oct 5, 2015 at 11:55 AM, Paul Irish notifications@github.com wrote:

@ericguzman https://github.com/ericguzman looks like your commit is to address our comments. should we take another look?

— Reply to this email directly or view it on GitHub https://github.com/ChromeDevTools/debugger-protocol-viewer/pull/34#issuecomment-145631419 .

ericguzman commented 9 years ago

@kdzwinel - Just a few more notes on code updates per your review:

  1. Took your suggestion regarding the search data optimization by adding link construction logic to the view. With a couple of other adjustments, I was able to bring the size down from ~150kb to ~115, which gzips to about 18kb.
  2. Hmm. I can’t repro this issue.
  3. Done.
kdzwinel commented 9 years ago

Took your suggestion regarding the search data optimization by adding link construction logic to the view. With a couple of other adjustments, I was able to bring the size down from ~150kb to ~115, which gzips to about 18kb.

Sweet!

2 Hmm. I can’t repro this issue.

I'm able to reproduce it every time: https://dl.dropboxusercontent.com/u/8989748/font.mov


There are no updates to the searchindex.json after merging upstream changes? https://github.com/ericguzman/debugger-protocol-viewer/commit/a7b93faac7104f1215e1a1de34d6a53e6c9612b7

kdzwinel commented 9 years ago

@ericguzman FYI I'm absolutely OK with merging that PR after you updated the search index. Double font loading issue is small and can be resolved later on.

paulirish commented 9 years ago

did https://github.com/ericguzman/debugger-protocol-viewer/commit/41791f1d80c41fe305a725df4e4a42826a1a56e1 update the index?

ericguzman commented 9 years ago

@paulirish Yup.

I've created an issue for fixing the double font load issue. I found that I couldn't repro on my machines, presumably because I have Roboto installed locally on them. Using a linux box at work showed the issue, and I have a couple things to try next. Thanks for the vid, @kdzwinel.

So we're good to go with this PR now?