Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
918913002:
Add a <cr-input> element to cr_elements. (Closed)
|
Created:
5 years, 10 months ago by Jeremy Klein Modified:
5 years, 9 months ago CC:
chromium-reviews, khorimoto+watch-md-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, jhawkins+watch-md-settings_chromium.org, orenb+watch-md-settings_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-md-settings_chromium.org, ajo, tjsavage_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a <cr-input> element to cr_elements.
BUG=
Committed: https://crrev.com/c830ceceb0d71f9a66ff4bc1de75d3566d06a1d1
Cr-Commit-Position: refs/heads/master@{#318662}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 24
Patch Set 4 : Address Michael's first round of comments." #
Total comments: 2
Patch Set 5 : Addressed line-length issues. #
Total comments: 19
Patch Set 6 : Address style comments #Patch Set 7 : Remove @method #
Total comments: 2
Patch Set 8 : Rebase and fix conflicts #Patch Set 9 : Add a license header to cr_input.css #
Messages
Total messages: 27 (6 generated)
jlklein@chromium.org changed reviewers: + dbeam@chromium.org, michaelpg@chromium.org, stevenjb@chromium.org
On 2015/02/12 23:39:24, Jeremy Klein wrote: > mailto:jlklein@chromium.org changed reviewers: > + mailto:dbeam@chromium.org, mailto:michaelpg@chromium.org, mailto:stevenjb@chromium.org Let's move the <form> discussion here. If we really want form support here, I'll probably need to switch this to be a warpper around the decorator instead and the user will need to provide their own core-input.
style comments https://codereview.chromium.org/918913002/diff/40001/chrome/browser/resources... File chrome/browser/resources/md_settings/md_settings.html (right): https://codereview.chromium.org/918913002/diff/40001/chrome/browser/resources... chrome/browser/resources/md_settings/md_settings.html:8: <link rel="import" href="chrome://resources/cr_elements/cr_input/cr_input.html"> nit: let's ask dbeam whether the 80-char limit applies here. http://www.chromium.org/developers/web-development-style-guide seems to imply that it doesn't. https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/cr_input.html (right): https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.html:5: <link rel="import" href="chrome://resources/cr_elements/cr_events/cr_events.html"> nit: let's be consistent in wrapping https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.html:12: <paper-input-decorator id="decorator" label="{{label}}" floatingLabel="{{floatingLabel}}" nit: 12, 14, 15 should be limited to 80 chars wide https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.html:12: <paper-input-decorator id="decorator" label="{{label}}" floatingLabel="{{floatingLabel}}" we use {{ spaces }} inside data-binding braces https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/cr_input.js (right): https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:8: * element composed of a`paper-input-decorator` and a `input is="core-input". missing space: a `paper-input-decorator` missing closing `: and a `input is="core-input"`. https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:61: * @type String lower case https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:70: * @type String lower case https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:79: * @type String lower case https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:98: * @type String lower case https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/demo.html (right): https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/demo.html:1: <html> <!doctype html>
https://codereview.chromium.org/918913002/diff/40001/chrome/browser/resources... File chrome/browser/resources/md_settings/md_settings.html (right): https://codereview.chromium.org/918913002/diff/40001/chrome/browser/resources... chrome/browser/resources/md_settings/md_settings.html:8: <link rel="import" href="chrome://resources/cr_elements/cr_input/cr_input.html"> On 2015/02/13 00:08:28, michaelpg wrote: > nit: let's ask dbeam whether the 80-char limit applies here. > > http://www.chromium.org/developers/web-development-style-guide seems to imply > that it doesn't. I thought it was 100 for html, but it could be different for Chrome style. https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/cr_input.html (right): https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.html:5: <link rel="import" href="chrome://resources/cr_elements/cr_events/cr_events.html"> On 2015/02/13 00:08:29, michaelpg wrote: > nit: let's be consistent in wrapping Done. https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.html:12: <paper-input-decorator id="decorator" label="{{label}}" floatingLabel="{{floatingLabel}}" On 2015/02/13 00:08:28, michaelpg wrote: > nit: 12, 14, 15 should be limited to 80 chars wide Waiting on verification about 80-char limit. If we need it, I'll also need to change the imports. https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.html:12: <paper-input-decorator id="decorator" label="{{label}}" floatingLabel="{{floatingLabel}}" On 2015/02/13 00:08:29, michaelpg wrote: > we use {{ spaces }} inside data-binding braces Where did this convention come from? This isn't done in any of the core/paper elements. Happy to change it, but when was this decision made? https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/cr_input.js (right): https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:8: * element composed of a`paper-input-decorator` and a `input is="core-input". On 2015/02/13 00:08:29, michaelpg wrote: > missing space: a `paper-input-decorator` > missing closing `: and a `input is="core-input"`. Done. https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:61: * @type String On 2015/02/13 00:08:29, michaelpg wrote: > lower case I took this from the Polymer elements docs. Apparently there's one more place where the docs disagree (for now). I'll switch these all to lowercase though. https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:70: * @type String On 2015/02/13 00:08:29, michaelpg wrote: > lower case Done. https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:79: * @type String On 2015/02/13 00:08:29, michaelpg wrote: > lower case Done. https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:98: * @type String On 2015/02/13 00:08:29, michaelpg wrote: > lower case Done. https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/demo.html (right): https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/demo.html:1: <html> On 2015/02/13 00:08:29, michaelpg wrote: > <!doctype html> Done.
https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/cr_input.html (right): https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.html:12: <paper-input-decorator id="decorator" label="{{label}}" floatingLabel="{{floatingLabel}}" On 2015/02/13 00:22:54, Jeremy Klein wrote: > On 2015/02/13 00:08:29, michaelpg wrote: > > we use {{ spaces }} inside data-binding braces > > Where did this convention come from? This isn't done in any of the core/paper > elements. Happy to change it, but when was this decision made? Hmm, I guess the truth is *I* use {{ spaces }} inside the braces. I guess we should stick with the Polymer style of not using spaces, so this is good as-is. 1. Polymer files (third_party/Polymer/) don't use spaces: {{label}} 2. non-infra Chrome doesn't use spaces: {{label}} (19 files out of 23, not counting cr-toggle-button for now) 3. sheriff-o-matic uses spaces: {{ label }} (17 files out of 21) Personally I think spaces are more readable, and I carried that style over from sheriff-o-matic without realizing other elements in Chrome didn't use spaces. But it's more important that we stay consistent with 1) and 2) than with 3) and it doesn't bother me too much. [1] https://code.google.com/p/chromium/codesearch#search/&q=polymer%20file:.html%... [2] https://code.google.com/p/chromium/codesearch#search/&q=polymer%20file:.html%... [3] https://code.google.com/p/chromium/codesearch#search/&q=polymer%20file:.html%...
On 2015/02/13 05:45:13, michaelpg wrote: > https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... > File ui/webui/resources/cr_elements/cr_input/cr_input.html (right): > > https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... > ui/webui/resources/cr_elements/cr_input/cr_input.html:12: <paper-input-decorator > id="decorator" label="{{label}}" floatingLabel="{{floatingLabel}}" > On 2015/02/13 00:22:54, Jeremy Klein wrote: > > On 2015/02/13 00:08:29, michaelpg wrote: > > > we use {{ spaces }} inside data-binding braces > > > > Where did this convention come from? This isn't done in any of the core/paper > > elements. Happy to change it, but when was this decision made? > > Hmm, I guess the truth is *I* use {{ spaces }} inside the braces. I guess we > should stick with the Polymer style of not using spaces, so this is good as-is. > > 1. Polymer files (third_party/Polymer/) don't use spaces: {{label}} > > 2. non-infra Chrome doesn't use spaces: {{label}} (19 files out of 23, not > counting cr-toggle-button for now) > > 3. sheriff-o-matic uses spaces: {{ label }} (17 files out of 21) > > Personally I think spaces are more readable, and I carried that style over from > sheriff-o-matic without realizing other elements in Chrome didn't use spaces. > But it's more important that we stay consistent with 1) and 2) than with 3) and > it doesn't bother me too much. > > [1] > https://code.google.com/p/chromium/codesearch#search/&q=polymer%20file:.html%... > > [2] > https://code.google.com/p/chromium/codesearch#search/&q=polymer%20file:.html%... > > [3] > https://code.google.com/p/chromium/codesearch#search/&q=polymer%20file:.html%... I don't mind either way. I do see the readability benefit from the spaces. but also like the consistency with other web components. To me, it's really a wash.
https://codereview.chromium.org/918913002/diff/40001/chrome/browser/resources... File chrome/browser/resources/md_settings/md_settings.html (right): https://codereview.chromium.org/918913002/diff/40001/chrome/browser/resources... chrome/browser/resources/md_settings/md_settings.html:8: <link rel="import" href="chrome://resources/cr_elements/cr_input/cr_input.html"> On 2015/02/13 00:22:54, Jeremy Klein wrote: > On 2015/02/13 00:08:28, michaelpg wrote: > > nit: let's ask dbeam whether the 80-char limit applies here. > > > > http://www.chromium.org/developers/web-development-style-guide seems to imply > > that it doesn't. > > I thought it was 100 for html, but it could be different for Chrome style. It's definitely 80 for html in Chromium. I'd still like dbeam@ to weigh in on how we should handle long lines in <head>. https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/cr_input.html (right): https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.html:12: <paper-input-decorator id="decorator" label="{{label}}" floatingLabel="{{floatingLabel}}" On 2015/02/13 00:08:28, michaelpg wrote: > nit: 12, 14, 15 should be limited to 80 chars wide ping on this nit https://codereview.chromium.org/918913002/diff/60001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/demo.html (right): https://codereview.chromium.org/918913002/diff/60001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/demo.html:1: <!DOCTYPE html> nit: since this is case-insensitive and we use lowercase for tag names, we tend to use <!doctype html> in Chrome
Just waiting on Dan's input then for the html import wrapping. https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/cr_input.html (right): https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.html:12: <paper-input-decorator id="decorator" label="{{label}}" floatingLabel="{{floatingLabel}}" On 2015/02/18 00:17:18, michaelpg wrote: > On 2015/02/13 00:08:28, michaelpg wrote: > > nit: 12, 14, 15 should be limited to 80 chars wide > > ping on this nit Done. https://codereview.chromium.org/918913002/diff/60001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/demo.html (right): https://codereview.chromium.org/918913002/diff/60001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/demo.html:1: <!DOCTYPE html> On 2015/02/18 00:17:18, michaelpg wrote: > nit: since this is case-insensitive and we use lowercase for tag names, we tend > to use <!doctype html> in Chrome Done.
On 2015/02/18 01:10:50, Jeremy Klein wrote: > Just waiting on Dan's input then for the html import wrapping. > > https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... > File ui/webui/resources/cr_elements/cr_input/cr_input.html (right): > > https://codereview.chromium.org/918913002/diff/40001/ui/webui/resources/cr_el... > ui/webui/resources/cr_elements/cr_input/cr_input.html:12: <paper-input-decorator > id="decorator" label="{{label}}" floatingLabel="{{floatingLabel}}" > On 2015/02/18 00:17:18, michaelpg wrote: > > On 2015/02/13 00:08:28, michaelpg wrote: > > > nit: 12, 14, 15 should be limited to 80 chars wide > > > > ping on this nit > > Done. > > https://codereview.chromium.org/918913002/diff/60001/ui/webui/resources/cr_el... > File ui/webui/resources/cr_elements/cr_input/demo.html (right): > > https://codereview.chromium.org/918913002/diff/60001/ui/webui/resources/cr_el... > ui/webui/resources/cr_elements/cr_input/demo.html:1: <!DOCTYPE html> > On 2015/02/18 00:17:18, michaelpg wrote: > > nit: since this is case-insensitive and we use lowercase for tag names, we > tend > > to use <!doctype html> in Chrome > > Done. Consensus on the email thread seems to at least be to ignore the 80-col limit on imports, so I think I'm good here.
https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/cr_input.js (right): https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:7: * 'cr-input` is a single-line text field for user input. It is a convenience why ' + ` ? https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:8: * element composed of a `paper-input-decorator` and a `input is="core-input". think you're missing a ` here https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:35: * @default true why do we have all these duplicate @defaults? https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:90: * @type string why no @default? or conversely, why not no @default everywhere? :P https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:107: * @method focus useless https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:118: if (this.pattern) { nit: most of webui has no curlies https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:124: /** @override */ on all these? https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/demo.html (right): https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/demo.html:9: <cr-input label="Only numbers" pattern="^[0-9]*$" error="Only numbers allowed!"></cr-input> not an import
Taylor and AJ - please check out the two doc questions regarding @method and @default. https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/cr_input.js (right): https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:7: * 'cr-input` is a single-line text field for user input. It is a convenience On 2015/02/20 23:45:46, Dan Beam wrote: > why ' + ` ? Ha, good catch. The back ticks are totally copy-pasta. Changed to single-quotes everywhere. https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:8: * element composed of a `paper-input-decorator` and a `input is="core-input". On 2015/02/20 23:45:46, Dan Beam wrote: > think you're missing a ` here Done. https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:35: * @default true On 2015/02/20 23:45:47, Dan Beam wrote: > why do we have all these duplicate @defaults? Yeah I also feel like this is excessive, but for the time being that's how the docs are in polymer for the generator. +ajo and +tjsavage for input on whether we can eliminate the @default docs for 0.8. I agree that they should totally be unnecessary. https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:90: * @type string On 2015/02/20 23:45:46, Dan Beam wrote: > why no @default? or conversely, why not no @default everywhere? :P Done. https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:107: * @method focus On 2015/02/20 23:45:46, Dan Beam wrote: > useless Yeah I hate this doc. Removed for now since we don't seem to have it in other elements, but cc-ing ajo@ to see if we actually need these too. https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:118: if (this.pattern) { On 2015/02/20 23:45:46, Dan Beam wrote: > nit: most of webui has no curlies Done. https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:124: On 2015/02/20 23:45:46, Dan Beam wrote: > /** @override */ on all these? Done for ready. Not really sure what to put for the *Changed functions because they aren't actually overriding anything. The doc would just be "called when pattern changes", which I don't really think adds any value. WDYT? https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/demo.html (right): https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/demo.html:9: <cr-input label="Only numbers" pattern="^[0-9]*$" error="Only numbers allowed!"></cr-input> On 2015/02/20 23:45:47, Dan Beam wrote: > not an import OK, you got me here lol. Done.
https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/cr_input.js (right): https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:7: * 'cr-input` is a single-line text field for user input. It is a convenience On 2015/02/20 23:59:21, Jeremy Klein wrote: > On 2015/02/20 23:45:46, Dan Beam wrote: > > why ' + ` ? > > Ha, good catch. The back ticks are totally copy-pasta. Changed to single-quotes > everywhere. we've been using `s because the polymer doc generator interprets those as code, markdown-style. But we don't use them in other chrome JS. https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:107: * @method focus On 2015/02/20 23:59:21, Jeremy Klein wrote: > On 2015/02/20 23:45:46, Dan Beam wrote: > > useless > > Yeah I hate this doc. Removed for now since we don't seem to have it in other > elements, but cc-ing ajo@ to see if we actually need these too. :-\ this indicates which methods are considered "public" (and show up in core-doc-viewer) we could use @private instead... but that would involve more work for core-doc-viewer (context-free-parser) https://codereview.chromium.org/918913002/diff/120001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_input/demo.html (right): https://codereview.chromium.org/918913002/diff/120001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_input/demo.html:10: <cr-input label="Only numbers" pattern="^[0-9]*$" nit/question... does "^\d*$" not work?
https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_input/cr_input.js (right): https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_input/cr_input.js:35: * @default true On 2015/02/20 23:59:21, Jeremy Klein wrote: > On 2015/02/20 23:45:47, Dan Beam wrote: > > why do we have all these duplicate @defaults? > > Yeah I also feel like this is excessive, but for the time being that's how the > docs are in polymer for the generator. > > +ajo and +tjsavage for input on whether we can eliminate the @default docs for > 0.8. I agree that they should totally be unnecessary. Just revisited the 0.8 primer (which seems to have changed a fair amount since I last looked) and it looks like @default will go away then: https://github.com/Polymer/polymer/blob/0.8-preview/PRIMER.md#configure-callback Really the whole process by which these properties get declared and initialized is changing so personally, I don't care whether we keep the @defaults now. Anyone else have a strong opinion about this?
On 2015/02/21 00:15:37, Jeremy Klein wrote: > https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... > File ui/webui/resources/cr_elements/cr_input/cr_input.js (right): > > https://codereview.chromium.org/918913002/diff/80001/ui/webui/resources/cr_el... > ui/webui/resources/cr_elements/cr_input/cr_input.js:35: * @default true > On 2015/02/20 23:59:21, Jeremy Klein wrote: > > On 2015/02/20 23:45:47, Dan Beam wrote: > > > why do we have all these duplicate @defaults? > > > > Yeah I also feel like this is excessive, but for the time being that's how the > > docs are in polymer for the generator. > > > > +ajo and +tjsavage for input on whether we can eliminate the @default docs for > > 0.8. I agree that they should totally be unnecessary. > > Just revisited the 0.8 primer (which seems to have changed a fair amount since I > last looked) and it looks like @default will go away then: > > https://github.com/Polymer/polymer/blob/0.8-preview/PRIMER.md#configure-callback > > Really the whole process by which these properties get declared and initialized > is changing so personally, I don't care whether we keep the @defaults now. > Anyone else have a strong opinion about this? > The configure method is part of an element's lifecycle and is automatically called 'top-down' and should be used to initialize default values for properties. I like this! I've encountered some *really* weird bugs with Polymer assuming things about attributes I didn't specify a default for. And this prevents us from assigning an object to attributes on the prototype instead of the instance.
-dbeam
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
@default can go away for 0.8 as long as there's an explicit default in the publish block or on the element's prototype. Our doc/extern generator has basic knowledge of the element declaration in cases like that. However, if you get fancy enough, my static analysis will break down and you'll need to use it again. Examples of getting fancy: setting a default with an identifier instead of a consant(although support for this will come soon) nested closures(also planned to be supported) On Fri, Feb 20, 2015 at 4:56 PM, <dbeam@chromium.org> wrote: > -dbeam > > https://codereview.chromium.org/918913002/ > -- AJ Ortega | Software Engineer | ajo@google.com | 626-872-5064 To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
lgtm https://codereview.chromium.org/918913002/diff/120001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/cr_input/cr_input.js (right): https://codereview.chromium.org/918913002/diff/120001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/cr_input/cr_input.js:111: nit: FWIW, I still think that it would be good to establish a comment pattern for fooChanged methods to make them easier to identify and provide a hint for developers new to Polymer scanning this code.
The CQ bit was checked by jlklein@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/918913002/#ps140001 (title: "Rebase and fix conflicts")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/918913002/140001
The CQ bit was checked by jlklein@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/918913002/#ps160001 (title: "Add a license header to cr_input.css")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/918913002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c830ceceb0d71f9a66ff4bc1de75d3566d06a1d1 Cr-Commit-Position: refs/heads/master@{#318662} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
