Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10809062:
Extensions Docs Server: Samples search and icons (Closed)
|
Created:
8 years, 5 months ago by cduvall Modified:
8 years, 5 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, chebert, clintstaley Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionExtensions Docs Server: Samples search and icons
Added the search feature to the samples page, and also added icons next to the
samples.
BUG=131095
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148260
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148416
Patch Set 1 #
Total comments: 1
Patch Set 2 : Copyright #
Total comments: 21
Patch Set 3 : fixes #
Total comments: 4
Patch Set 4 : more fixes #
Total comments: 2
Patch Set 5 : Why use update when you can use []! #Patch Set 6 : license #Patch Set 7 : Commit again to fix static path for non local docs #
Messages
Total messages: 7 (0 generated)
Samples page getting close to done! https://chromiumcodereview.appspot.com/10809062/diff/1/chrome/common/extensio... File chrome/common/extensions/docs/server2/static/js/sample_search.js (right): https://chromiumcodereview.appspot.com/10809062/diff/1/chrome/common/extensio... chrome/common/extensions/docs/server2/static/js/sample_search.js:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. Use of this I just rewrote this file because the old one seemed like it was unnecessarily long/complicated. And this one actually works! That being said, there might be some features that the old one used to have that this doesn't, but I didn't see any.
sweet https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... File chrome/common/extensions/docs/server2/static/css/samples.css (right): https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... chrome/common/extensions/docs/server2/static/css/samples.css:1: #controls { general CSS comment: if you look at the page there is a lot of underlining going on. Distracting underlining, like underneath the headers, and for each API name. Then the links in "Filter by API" aren't underlined and it looks like a crazy block of text. Here's what I think: get rid of all the underlines, then see comment in the template. https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... chrome/common/extensions/docs/server2/static/css/samples.css:14: position: absolute; Yeah, really weird to be using the type of tag to do positioning. Definitely calls for classed-spans. Also, perhaps using a table would be more appropriate than this manual positioning logic. https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... File chrome/common/extensions/docs/server2/static/js/sample_search.js (right): https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... chrome/common/extensions/docs/server2/static/js/sample_search.js:17: apis[i].style.display = 'block'; This assumes that it was block. It might not be? Set to '' instead. https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... chrome/common/extensions/docs/server2/static/js/sample_search.js:27: window.setFilter = setFilter; see comment in the template. Use search_box.addEventListener and whatever_other_thing_is_called.addEventListener. https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... File chrome/common/extensions/docs/server2/templates/public/samples.html (right): https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... chrome/common/extensions/docs/server2/templates/public/samples.html:15: <strong>Filter by keyword:</strong> we should be using styles rather than <strong> https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... chrome/common/extensions/docs/server2/templates/public/samples.html:16: <input autofocus="" type="search" id="search_input" placeholder="Type to search" onsearch="filterSamples();" onkeyup="filterSamples();"> Having events declarative in here is nice, because it's declarative. However it necessitates polluting the global namespace with methods; sustainable now, but if this grows then it'd get unsustainable. So rather than this, in sample_search.js grab the element then use addEventListener('search', filerSamples) and addEventListener('keyup', filterSamples), where filterSamples is defined inside that anonymous function(){} and not on window. https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... chrome/common/extensions/docs/server2/templates/public/samples.html:19: <strong>Filter by API:</strong> ditto https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... chrome/common/extensions/docs/server2/templates/public/samples.html:21: <span><a href="javascript:void(0);" onclick="setFilter('chrome.{{@}}')">chrome.{{@}}</a> </span> ditto btw, one other thing with this case: you'd be better off putting a single event listener on the div that contains all these <a>s, then checking the event target and either using its innerText or having some custom attribute on the HTML (innerText should be fine). https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... chrome/common/extensions/docs/server2/templates/public/samples.html:22: {{/}} Also, this generated HTML that looks like chrome.alarms chrome.appWindow chrome.bookmarks chrome.browserAction chrome.browsingData chrome.contentSettings chrome.contextMenus chrome.cookies chrome.debugger chrome.declarativeWebRequest chrome.devtools chrome.downloads chrome.extension chrome.fileBrowserHandler chrome.fileSystem chrome.fontSettings chrome.history chrome.i18n chrome.idle chrome.input.ime chrome.management chrome.omnibox chrome.pageAction chrome.pageCapture chrome.permissions chrome.privacy chrome.proxy chrome.runtime chrome.scriptBadge chrome.storage chrome.tabs chrome.topSites chrome.tts chrome.ttsEngine chrome.types chrome.webNavigation chrome.webRequest chrome.webstore chrome.windows which is really hard to parse. By a human I mean. Better would be (and I just tried this) alarms | appWindow | bookmarks | browserAction | browsingData | contentSettings | contextMenus | cookies | debugger | declarativeWebRequest | devtools | downloads | extension | fileBrowserHandler | fileSystem | fontSettings | history | i18n | idle | input.ime | management | omnibox | pageAction | pageCapture | permissions | privacy | proxy | runtime | scriptBadge | storage | tabs | topSites | tts | ttsEngine | types | webNavigation | webRequest | webstore | windows so we should do that. https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... chrome/common/extensions/docs/server2/templates/public/samples.html:27: <img class="icon" src="{{?icon}}{{path}}/{{icon}}{{:}}{{static}}/images/sample-default-icon.png{{/}}"> this logic could be moved into the data source? Like, just set the icon here. Data source knows that if there isn't a 128 icon, use the default path. https://chromiumcodereview.appspot.com/10809062/diff/4001/chrome/common/exten... chrome/common/extensions/docs/server2/templates/public/samples.html:52: <script src="{{static}}/js/sample_search.js" type="text/javascript"></script> just samples.js? Match the name of the HTML/CSS?
http://codereview.chromium.org/10809062/diff/4001/chrome/common/extensions/do... File chrome/common/extensions/docs/server2/static/css/samples.css (right): http://codereview.chromium.org/10809062/diff/4001/chrome/common/extensions/do... chrome/common/extensions/docs/server2/static/css/samples.css:1: #controls { On 2012/07/24 01:09:55, kalman wrote: > general CSS comment: if you look at the page there is a lot of underlining going > on. Distracting underlining, like underneath the headers, and for each API name. > > Then the links in "Filter by API" aren't underlined and it looks like a crazy > block of text. > > Here's what I think: get rid of all the underlines, then see comment in the > template. Done. I assume all the underlining you were talking about is in the old samples page? http://codereview.chromium.org/10809062/diff/4001/chrome/common/extensions/do... chrome/common/extensions/docs/server2/static/css/samples.css:14: position: absolute; On 2012/07/24 01:09:55, kalman wrote: > Yeah, really weird to be using the type of tag to do positioning. Definitely > calls for classed-spans. > > Also, perhaps using a table would be more appropriate than this manual > positioning logic. Done. http://codereview.chromium.org/10809062/diff/4001/chrome/common/extensions/do... File chrome/common/extensions/docs/server2/static/js/sample_search.js (right): http://codereview.chromium.org/10809062/diff/4001/chrome/common/extensions/do... chrome/common/extensions/docs/server2/static/js/sample_search.js:17: apis[i].style.display = 'block'; On 2012/07/24 01:09:55, kalman wrote: > This assumes that it was block. It might not be? Set to '' instead. Done. http://codereview.chromium.org/10809062/diff/4001/chrome/common/extensions/do... chrome/common/extensions/docs/server2/static/js/sample_search.js:27: window.setFilter = setFilter; On 2012/07/24 01:09:55, kalman wrote: > see comment in the template. Use search_box.addEventListener and > whatever_other_thing_is_called.addEventListener. Done. http://codereview.chromium.org/10809062/diff/4001/chrome/common/extensions/do... File chrome/common/extensions/docs/server2/templates/public/samples.html (right): http://codereview.chromium.org/10809062/diff/4001/chrome/common/extensions/do... chrome/common/extensions/docs/server2/templates/public/samples.html:15: <strong>Filter by keyword:</strong> On 2012/07/24 01:09:55, kalman wrote: > we should be using styles rather than <strong> Done. http://codereview.chromium.org/10809062/diff/4001/chrome/common/extensions/do... chrome/common/extensions/docs/server2/templates/public/samples.html:16: <input autofocus="" type="search" id="search_input" placeholder="Type to search" onsearch="filterSamples();" onkeyup="filterSamples();"> On 2012/07/24 01:09:55, kalman wrote: > Having events declarative in here is nice, because it's declarative. However it > necessitates polluting the global namespace with methods; sustainable now, but > if this grows then it'd get unsustainable. > > So rather than this, in sample_search.js grab the element then use > addEventListener('search', filerSamples) and addEventListener('keyup', > filterSamples), where filterSamples is defined inside that anonymous > function(){} and not on window. Done. http://codereview.chromium.org/10809062/diff/4001/chrome/common/extensions/do... chrome/common/extensions/docs/server2/templates/public/samples.html:19: <strong>Filter by API:</strong> On 2012/07/24 01:09:55, kalman wrote: > ditto Done. http://codereview.chromium.org/10809062/diff/4001/chrome/common/extensions/do... chrome/common/extensions/docs/server2/templates/public/samples.html:21: <span><a href="javascript:void(0);" onclick="setFilter('chrome.{{@}}')">chrome.{{@}}</a> </span> On 2012/07/24 01:09:55, kalman wrote: > ditto > > btw, one other thing with this case: you'd be better off putting a single event > listener on the div that contains all these <a>s, then checking the event target > and either using its innerText or having some custom attribute on the HTML > (innerText should be fine). Putting the listener on the outer div was giving me some problems because, (1) The target given for the event is the href of the link, not the <a> element itself, and (2) clicks not on the <a> elements also triggered the event. I don't have all that much experience with JS, so maybe I was doing it wrong? http://codereview.chromium.org/10809062/diff/4001/chrome/common/extensions/do... chrome/common/extensions/docs/server2/templates/public/samples.html:22: {{/}} On 2012/07/24 01:09:55, kalman wrote: > Also, this generated HTML that looks like > > chrome.alarms chrome.appWindow chrome.bookmarks chrome.browserAction > chrome.browsingData chrome.contentSettings chrome.contextMenus chrome.cookies > chrome.debugger chrome.declarativeWebRequest chrome.devtools chrome.downloads > chrome.extension chrome.fileBrowserHandler chrome.fileSystem chrome.fontSettings > chrome.history chrome.i18n chrome.idle chrome.input.ime chrome.management > chrome.omnibox chrome.pageAction chrome.pageCapture chrome.permissions > chrome.privacy chrome.proxy chrome.runtime chrome.scriptBadge chrome.storage > chrome.tabs chrome.topSites chrome.tts chrome.ttsEngine chrome.types > chrome.webNavigation chrome.webRequest chrome.webstore chrome.windows > > which is really hard to parse. By a human I mean. Better would be (and I just > tried this) > > alarms | appWindow | bookmarks | browserAction | browsingData | contentSettings > | contextMenus | cookies | debugger | declarativeWebRequest | devtools | > downloads | extension | fileBrowserHandler | fileSystem | fontSettings | history > | i18n | idle | input.ime | management | omnibox | pageAction | pageCapture | > permissions | privacy | proxy | runtime | scriptBadge | storage | tabs | > topSites | tts | ttsEngine | types | webNavigation | webRequest | webstore | > windows > > so we should do that. Done. http://codereview.chromium.org/10809062/diff/4001/chrome/common/extensions/do... chrome/common/extensions/docs/server2/templates/public/samples.html:27: <img class="icon" src="{{?icon}}{{path}}/{{icon}}{{:}}{{static}}/images/sample-default-icon.png{{/}}"> On 2012/07/24 01:09:55, kalman wrote: > this logic could be moved into the data source? Like, just set the icon here. > Data source knows that if there isn't a 128 icon, use the default path. Done.
Here is what I meant with the addEventListener stuff: http://codereview.chromium.org/10824003/ http://codereview.chromium.org/10809062/diff/1007/chrome/common/extensions/do... File chrome/common/extensions/docs/server2/static/css/samples.css (right): http://codereview.chromium.org/10809062/diff/1007/chrome/common/extensions/do... chrome/common/extensions/docs/server2/static/css/samples.css:9: position: relative; this doesn't need to be positioned (none of its children are positioned), so you can delete the class. http://codereview.chromium.org/10809062/diff/1007/chrome/common/extensions/do... File chrome/common/extensions/docs/server2/templates/public/samples.html (right): http://codereview.chromium.org/10809062/diff/1007/chrome/common/extensions/do... chrome/common/extensions/docs/server2/templates/public/samples.html:23: <span><a class="api_filter_item" href="javascript:void(0)">{{@}}</a> | </span> this needs the {{^last}} | {{/last}} treatment.
Thanks for the JS/CSS/HTML help :) http://codereview.chromium.org/10809062/diff/1007/chrome/common/extensions/do... File chrome/common/extensions/docs/server2/static/css/samples.css (right): http://codereview.chromium.org/10809062/diff/1007/chrome/common/extensions/do... chrome/common/extensions/docs/server2/static/css/samples.css:9: position: relative; On 2012/07/24 23:47:02, kalman wrote: > this doesn't need to be positioned (none of its children are positioned), so you > can delete the class. Done. http://codereview.chromium.org/10809062/diff/1007/chrome/common/extensions/do... File chrome/common/extensions/docs/server2/templates/public/samples.html (right): http://codereview.chromium.org/10809062/diff/1007/chrome/common/extensions/do... chrome/common/extensions/docs/server2/templates/public/samples.html:23: <span><a class="api_filter_item" href="javascript:void(0)">{{@}}</a> | </span> On 2012/07/24 23:47:02, kalman wrote: > this needs the {{^last}} | {{/last}} treatment. Done.
lgtm http://codereview.chromium.org/10809062/diff/16001/chrome/common/extensions/d... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): http://codereview.chromium.org/10809062/diff/16001/chrome/common/extensions/d... chrome/common/extensions/docs/server2/api_list_data_source.py:32: experimental_apis[-1].update({ 'last': True }) just curious, is there some reason you did this rather than just chrome_apis[-1]['last'] = True?
https://chromiumcodereview.appspot.com/10809062/diff/16001/chrome/common/exte... File chrome/common/extensions/docs/server2/api_list_data_source.py (right): https://chromiumcodereview.appspot.com/10809062/diff/16001/chrome/common/exte... chrome/common/extensions/docs/server2/api_list_data_source.py:32: experimental_apis[-1].update({ 'last': True }) On 2012/07/25 00:18:08, kalman wrote: > just curious, is there some reason you did this rather than just > chrome_apis[-1]['last'] = True? I have no idea why I decided to use update :) Changed it to [] |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
