Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
902053003:
Add cr-toggle-button to Chrome (Closed)
DescriptionAdd cr-toggle-button to Chrome
Adds the chrome://resources/cr_elements URL, adding the
custom elements in ui/webui/resources/cr_elements. Sample in
chrome://md-settings.
Also creates cr-events to forward events that don't
cross the shadow root on their own.
BUG=456284
Committed: https://crrev.com/43dff8c48a305f4212a753d2afb58ed1cdbdbbd8
Cr-Commit-Position: refs/heads/master@{#315455}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Renames, expand cr-events, style cr-toggle-button #
Total comments: 5
Patch Set 3 : more renames; add to chrome://md-settings #
Total comments: 9
Patch Set 4 : change cr-events api #
Total comments: 10
Patch Set 5 : #hex->rgb #Patch Set 6 : jsdoc spacing #Patch Set 7 : closure linter fix #Messages
Total messages: 36 (7 generated)
Would you mind posting a screenshot?
I like the location of ui/webui/resources/custom_elements/cr/ better than ui/webui/resources/js/cr/polymer_elements/, but I think I would prefer cr_custom_elements and cr-custom-elements rather than just 'custom_elements' + a cr/ subdirectory. What else would we put under 'custom_elements' ? https://codereview.chromium.org/902053003/diff/1/content/browser/webui/shared... File content/browser/webui/shared_resources_data_source.cc (right): https://codereview.chromium.org/902053003/diff/1/content/browser/webui/shared... content/browser/webui/shared_resources_data_source.cc:32: {"../../webui/resources/custom_elements/", "custom-elements/"} This should probably come before third_party (or third_party should come first). Also, 'custom-elements' isn't very descriptive to me. I would prefer cr-custom-elements. https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... File ui/webui/resources/custom_element_resources.grdp (right): https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... ui/webui/resources/custom_element_resources.grdp:18: </grit-part> This is good, I do like putting these into a separate file. https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... File ui/webui/resources/custom_elements/cr/toggle-button/cr-toggle-button.js (right): https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... ui/webui/resources/custom_elements/cr/toggle-button/cr-toggle-button.js:41: }, It does seem a little tedious to have to re-publish and re-document these from paper-toggle-button. Is there a way to make closure happy without copying the comment blocks? At minimum we should indicate that these are being forwarded and not introduced.
https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... File ui/webui/resources/custom_element_resources.grdp (right): https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... ui/webui/resources/custom_element_resources.grdp:18: </grit-part> On 2015/02/05 23:36:21, stevenjb wrote: > This is good, I do like putting these into a separate file. Agreed. Good call, Michael. https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... File ui/webui/resources/custom_elements/cr/toggle-button/cr-toggle-button.js (right): https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... ui/webui/resources/custom_elements/cr/toggle-button/cr-toggle-button.js:41: }, On 2015/02/05 23:36:21, stevenjb wrote: > It does seem a little tedious to have to re-publish and re-document these from > paper-toggle-button. Is there a way to make closure happy without copying the > comment blocks? At minimum we should indicate that these are being forwarded and > not introduced. +1. This is a lot of code for a really thin wrapper, but we did see this coming :-(. Unfortunately, without a clear inheritance story, I don't see a way to reduce this boilerplate. Indicating that they're just delegates seems reasonable, though. I wonder if using mixins will eventually be the 0.8 way to do things like this. https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... File ui/webui/resources/custom_elements/cr/util/cr-util.js (right): https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... ui/webui/resources/custom_elements/cr/util/cr-util.js:13: * @element cr-util I'd like to keep this name a little bit more well-scoped so that it doesn't become a dumping ground. Maybe cr-events or even cr-event-forwarder? https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... ui/webui/resources/custom_elements/cr/util/cr-util.js:24: // TODO What should this TODO say?
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/902053003/diff/1/content/browser/webui/shared... File content/browser/webui/shared_resources_data_source.cc (right): https://codereview.chromium.org/902053003/diff/1/content/browser/webui/shared... content/browser/webui/shared_resources_data_source.cc:32: {"../../webui/resources/custom_elements/", "custom-elements/"} On 2015/02/05 23:36:21, stevenjb wrote: > This should probably come before third_party (or third_party should come first). > > Also, 'custom-elements' isn't very descriptive to me. I would prefer > cr-custom-elements. Done. https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... File ui/webui/resources/custom_elements/cr/util/cr-util.js (right): https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... ui/webui/resources/custom_elements/cr/util/cr-util.js:13: * @element cr-util On 2015/02/05 23:42:11, Jeremy Klein wrote: > I'd like to keep this name a little bit more well-scoped so that it doesn't > become a dumping ground. Maybe cr-events or even cr-event-forwarder? Done. https://codereview.chromium.org/902053003/diff/1/ui/webui/resources/custom_el... ui/webui/resources/custom_elements/cr/util/cr-util.js:24: // TODO On 2015/02/05 23:42:11, Jeremy Klein wrote: > What should this TODO say? Fixed
https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_element_resources.grdp (right): https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_element_resources.grdp:3: <structure name="IDR_CR_ELEMENTS_TOGGLE_BUTTON_CR_TOGGLE_BUTTON_CSS" The IDR name is long enough to seem unmanageable. Is it so because we are concerned about clashing?
It seems like the directory name should be: ui/webui/resources/cr_elements/cr_toggle_button/ i.e. use _ instead of - and include the cr_. If we want to reduce the number of sub-directories I would be OK with cr_elements/cr_buttons/ for all custom button types. https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_element_resources.grdp (right): https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_element_resources.grdp:3: <structure name="IDR_CR_ELEMENTS_TOGGLE_BUTTON_CR_TOGGLE_BUTTON_CSS" On 2015/02/06 15:19:10, James Hawkins wrote: > The IDR name is long enough to seem unmanageable. Is it so because we are > concerned about clashing? I would suggest that the CR_ELEMENTS_TOGGLE_BUTTON bit is unnecessary. We should name our custom elements uniquely / descriptively.
On 2015/02/06 17:50:30, stevenjb wrote: > It seems like the directory name should be: > ui/webui/resources/cr_elements/cr_toggle_button/ > > i.e. use _ instead of - and include the cr_. > > If we want to reduce the number of sub-directories I would be OK with > cr_elements/cr_buttons/ for all custom button types. > > https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... > File ui/webui/resources/cr_element_resources.grdp (right): > > https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... > ui/webui/resources/cr_element_resources.grdp:3: <structure > name="IDR_CR_ELEMENTS_TOGGLE_BUTTON_CR_TOGGLE_BUTTON_CSS" > On 2015/02/06 15:19:10, James Hawkins wrote: > > The IDR name is long enough to seem unmanageable. Is it so because we are > > concerned about clashing? > > I would suggest that the CR_ELEMENTS_TOGGLE_BUTTON bit is unnecessary. We should > name our custom elements uniquely / descriptively. Ok, seems like the hyphens are only being used for the 3rd-party polymer elements, if we want our elements to stick to Chrome convention we'll use underscores. > git grep "chrome://resources/" | grep "<.*_.*>" | wc -l 477 > git grep "chrome://resources/" | grep "<.*-.*>" | wc -l 27
On 2015/02/06 17:50:30, stevenjb wrote: > It seems like the directory name should be: > ui/webui/resources/cr_elements/cr_toggle_button/ > > i.e. use _ instead of - and include the cr_. > > If we want to reduce the number of sub-directories I would be OK with > cr_elements/cr_buttons/ for all custom button types. Changed directories to use _ instead of - and include the cr. Not sure how I feel about cr_buttons. Maybe cr_inputs. paper-toggle-button is more akin to paper-checkbox than paper-button. Or just keep everything in its own subdirectory. https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_element_resources.grdp (right): https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_element_resources.grdp:3: <structure name="IDR_CR_ELEMENTS_TOGGLE_BUTTON_CR_TOGGLE_BUTTON_CSS" On 2015/02/06 17:50:30, stevenjb wrote: > On 2015/02/06 15:19:10, James Hawkins wrote: > > The IDR name is long enough to seem unmanageable. Is it so because we are > > concerned about clashing? > > I would suggest that the CR_ELEMENTS_TOGGLE_BUTTON bit is unnecessary. We should > name our custom elements uniquely / descriptively. I thought it was just convention to use the path. We don't have to worry about clashing here.
On 2015/02/06 21:32:02, michaelpg wrote: > On 2015/02/06 17:50:30, stevenjb wrote: > > It seems like the directory name should be: > > ui/webui/resources/cr_elements/cr_toggle_button/ > > > > i.e. use _ instead of - and include the cr_. > > > > If we want to reduce the number of sub-directories I would be OK with > > cr_elements/cr_buttons/ for all custom button types. > > Changed directories to use _ instead of - and include the cr. > > Not sure how I feel about cr_buttons. Maybe cr_inputs. paper-toggle-button is > more akin to paper-checkbox than paper-button. Or just keep everything in its > own subdirectory. I'd prefer keeping elements in their own subdirectories to follow the polymer pattern there. > > https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... > File ui/webui/resources/cr_element_resources.grdp (right): > > https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... > ui/webui/resources/cr_element_resources.grdp:3: <structure > name="IDR_CR_ELEMENTS_TOGGLE_BUTTON_CR_TOGGLE_BUTTON_CSS" > On 2015/02/06 17:50:30, stevenjb wrote: > > On 2015/02/06 15:19:10, James Hawkins wrote: > > > The IDR name is long enough to seem unmanageable. Is it so because we are > > > concerned about clashing? > > > > I would suggest that the CR_ELEMENTS_TOGGLE_BUTTON bit is unnecessary. We > should > > name our custom elements uniquely / descriptively. > > I thought it was just convention to use the path. We don't have to worry about > clashing here.
lgtm https://codereview.chromium.org/902053003/diff/60001/chrome/browser/resources... File chrome/browser/resources/md_settings/md_settings.html (right): https://codereview.chromium.org/902053003/diff/60001/chrome/browser/resources... chrome/browser/resources/md_settings/md_settings.html:18: <p> Nit: Not that it matters at this point, but I don't think <p> is the right tag to use here. Maybe <div> or <section>?
https://codereview.chromium.org/902053003/diff/60001/chrome/browser/resources... File chrome/browser/resources/md_settings/md_settings.html (right): https://codereview.chromium.org/902053003/diff/60001/chrome/browser/resources... chrome/browser/resources/md_settings/md_settings.html:18: <p> On 2015/02/09 18:24:51, Jeremy Klein wrote: > Nit: Not that it matters at this point, but I don't think <p> is the right tag > to use here. Maybe <div> or <section>? Or just move this <p>...</p> block within the 'main' div above.
https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_element_resources.grdp (right): https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_element_resources.grdp:3: <structure name="IDR_CR_ELEMENTS_TOGGLE_BUTTON_CR_TOGGLE_BUTTON_CSS" On 2015/02/06 17:50:30, stevenjb wrote: > On 2015/02/06 15:19:10, James Hawkins wrote: > > The IDR name is long enough to seem unmanageable. Is it so because we are > > concerned about clashing? > > I would suggest that the CR_ELEMENTS_TOGGLE_BUTTON bit is unnecessary. We should > name our custom elements uniquely / descriptively. Thinking about this more, I would keep the CR_ELEMENTS, but remove the redundant TOGGLE_BUTTON. https://codereview.chromium.org/902053003/diff/60001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.js (right): https://codereview.chromium.org/902053003/diff/60001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.js:45: this.$.events.forwardEvent); Question: Is the <cr-events> element useful without adding this event listener? If not, should we add this snippit to the Example section of cr-events.js? https://codereview.chromium.org/902053003/diff/60001/ui/webui/resources/webui... File ui/webui/resources/webui_resources.grd (right): https://codereview.chromium.org/902053003/diff/60001/ui/webui/resources/webui... ui/webui/resources/webui_resources.grd:390: <part file="cr_element_resources.grdp" /> I think this should actually be cr_elements_resources.grdp, to match the cr_elements/ directory.
lgtm https://codereview.chromium.org/902053003/diff/60001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.js (right): https://codereview.chromium.org/902053003/diff/60001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.js:6: * @fileoverview Should we make it the convention to always put the docs in the js instead of the html? I'm fine with that, just wanted to note it so that we're consistent moving forward.
lgtm
New patchsets have been uploaded after l-g-t-m from jlklein@chromium.org
michaelpg@chromium.org changed required reviewers: + jhawkins@chromium.org
Hope to get this in today. https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_element_resources.grdp (right): https://codereview.chromium.org/902053003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_element_resources.grdp:3: <structure name="IDR_CR_ELEMENTS_TOGGLE_BUTTON_CR_TOGGLE_BUTTON_CSS" On 2015/02/09 18:33:00, stevenjb wrote: > On 2015/02/06 17:50:30, stevenjb wrote: > > On 2015/02/06 15:19:10, James Hawkins wrote: > > > The IDR name is long enough to seem unmanageable. Is it so because we are > > > concerned about clashing? > > > > I would suggest that the CR_ELEMENTS_TOGGLE_BUTTON bit is unnecessary. We > should > > name our custom elements uniquely / descriptively. > Thinking about this more, I would keep the CR_ELEMENTS, but remove the redundant > TOGGLE_BUTTON. > Done. https://codereview.chromium.org/902053003/diff/60001/chrome/browser/resources... File chrome/browser/resources/md_settings/md_settings.html (right): https://codereview.chromium.org/902053003/diff/60001/chrome/browser/resources... chrome/browser/resources/md_settings/md_settings.html:18: <p> On 2015/02/09 18:24:51, Jeremy Klein wrote: > Nit: Not that it matters at this point, but I don't think <p> is the right tag > to use here. Maybe <div> or <section>? Done. https://codereview.chromium.org/902053003/diff/60001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.js (right): https://codereview.chromium.org/902053003/diff/60001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.js:6: * @fileoverview On 2015/02/09 19:20:25, Jeremy Klein wrote: > Should we make it the convention to always put the docs in the js instead of the > html? I'm fine with that, just wanted to note it so that we're consistent moving > forward. I think that's the right thing to do for the sake of Closure compiling. https://codereview.chromium.org/902053003/diff/60001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.js:45: this.$.events.forwardEvent); On 2015/02/09 18:33:00, stevenjb wrote: > Question: Is the <cr-events> element useful without adding this event listener? > If not, should we add this snippit to the Example section of cr-events.js? no, yes, done https://codereview.chromium.org/902053003/diff/60001/ui/webui/resources/webui... File ui/webui/resources/webui_resources.grd (right): https://codereview.chromium.org/902053003/diff/60001/ui/webui/resources/webui... ui/webui/resources/webui_resources.grd:390: <part file="cr_element_resources.grdp" /> On 2015/02/09 18:33:00, stevenjb wrote: > I think this should actually be cr_elements_resources.grdp, to match the > cr_elements/ directory. Done. The plural sounds weird to me but this is how we do it in c/b/resources.
On 2015/02/05 23:22:22, James Hawkins wrote: > Would you mind posting a screenshot? http://i.imgur.com/duj2oOe.png I don't know why my colors are messed up, the cr-toggle-button is actually blue.
LGTM with nits. I'm still concerned about the need for cr-events, but I want to allow us to move forward and see if a better solution may emerge. https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_events/cr-events.js (right): https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_events/cr-events.js:31: /** nit: Double-blank lines above JSDoc blocks (though I don't think you need one above line 20). https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.css (right): https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.css:10: background-color: #ECEFF1; nit: Please use rgb. http://www.chromium.org/developers/web-development-style-guide https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.js (right): https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.js:30: /** nit: Double blank line above this JSDoc. https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/polym... File ui/webui/resources/polymer_resources.grdp (right): https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/polym... ui/webui/resources/polymer_resources.grdp:3: <structure name="IDR_POLYMER_CORE_A11Y_KEYS_CORE_A11Y_KEYS_EXTRACTED_JS" As a side note, I still think our (Chrome's) pattern of naming these is unnecessarily verbose, but this is precedence already set, and we would need to go over the tradeoffs when considering changing the pattern. Nothing to do for this comment.
lgtm
New patchsets have been uploaded after l-g-t-m from jhawkins@chromium.org,stevenjb@chromium.org
https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_events/cr-events.js (right): https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_events/cr-events.js:31: /** On 2015/02/09 22:24:34, James Hawkins wrote: > nit: Double-blank lines above JSDoc blocks (though I don't think you need one > above line 20). Hmm I haven't seen that style in other web ui, where is this coming from? https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.css (right): https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.css:10: background-color: #ECEFF1; On 2015/02/09 22:24:35, James Hawkins wrote: > nit: Please use rgb. > > http://www.chromium.org/developers/web-development-style-guide Done.
https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_events/cr-events.js (right): https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_events/cr-events.js:31: /** On 2015/02/09 22:40:04, michaelpg wrote: > On 2015/02/09 22:24:34, James Hawkins wrote: > > nit: Double-blank lines above JSDoc blocks (though I don't think you need one > > above line 20). > > Hmm I haven't seen that style in other web ui, where is this coming from? I forgot this isn't in the JS style guide, but the Closure extension to the style guide [1]. Just chatted with Jeremy, and for the sake of conforming to the compiler (once we implement support) and the linter (which we should also add support for), we should stick to the Closure style guide as well as JS style guide. [1] https://sites.google.com/a/google.com/closure/documentation/closure-style-guide
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/902053003/140001
On 2015/02/09 22:24:35, James Hawkins wrote: > LGTM with nits. > > I'm still concerned about the need for cr-events, but I want to allow us to move > forward and see if a better solution may emerge. I'm waiting to hear back from the Polymer folks about this. https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_events/cr-events.js (right): https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_events/cr-events.js:31: /** On 2015/02/09 22:46:23, James Hawkins wrote: > On 2015/02/09 22:40:04, michaelpg wrote: > > On 2015/02/09 22:24:34, James Hawkins wrote: > > > nit: Double-blank lines above JSDoc blocks (though I don't think you need > one > > > above line 20). > > > > Hmm I haven't seen that style in other web ui, where is this coming from? > > I forgot this isn't in the JS style guide, but the Closure extension to the > style guide [1]. > > Just chatted with Jeremy, and for the sake of conforming to the compiler (once > we implement support) and the linter (which we should also add support for), we > should stick to the Closure style guide as well as JS style guide. > > [1] > https://sites.google.com/a/google.com/closure/documentation/closure-style-guide Done. https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.js (right): https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_toggle_button/cr-toggle-button.js:30: /** On 2015/02/09 22:24:35, James Hawkins wrote: > nit: Double blank line above this JSDoc. Done.
https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_events/cr-events.js (right): https://codereview.chromium.org/902053003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_events/cr-events.js:31: /** On 2015/02/09 22:46:23, James Hawkins wrote: > On 2015/02/09 22:40:04, michaelpg wrote: > > On 2015/02/09 22:24:34, James Hawkins wrote: > > > nit: Double-blank lines above JSDoc blocks (though I don't think you need > one > > > above line 20). > > > > Hmm I haven't seen that style in other web ui, where is this coming from? > > I forgot this isn't in the JS style guide, but the Closure extension to the > style guide [1]. > > Just chatted with Jeremy, and for the sake of conforming to the compiler (once > we implement support) and the linter (which we should also add support for), we > should stick to the Closure style guide as well as JS style guide. > > [1] > https://sites.google.com/a/google.com/closure/documentation/closure-style-guide fwiw: I'd discourage this for consistency with other code, but we're rewriting it all anyways and parts of webui already do the \n\n thing
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/43dff8c48a305f4212a753d2afb58ed1cdbdbbd8 Cr-Commit-Position: refs/heads/master@{#315455}
Message was sent while issue was closed.
Based on comments from https://codereview.chromium.org/874283006/, should we do a follow-up patch replacing - with _ in filenames?
Message was sent while issue was closed.
On 2015/02/10 17:16:49, stevenjb wrote: > Based on comments from https://codereview.chromium.org/874283006/, should we do > a follow-up patch replacing - with _ in filenames? Yes, please!
Message was sent while issue was closed.
Patchset #8 (id:160001) has been deleted |
