Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
909313003:
Add cr-checkbox to Chrome (Closed)
|
Created:
5 years, 10 months ago by Oren Blasberg Modified:
5 years, 10 months ago CC:
chromium-reviews, khorimoto+watch-md-settings_chromium.org, michaelpg+watch-polymer_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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds the cr-checkbox wrapper element around paper-checkbox. Sample in chrome://md-settings.
BUG=457513
Committed: https://crrev.com/1493ca1c409b970874c69095e89b39edbfae2882
Cr-Commit-Position: refs/heads/master@{#317190}
Patch Set 1 #Patch Set 2 : Update styling and use underscores #
Total comments: 14
Patch Set 3 : Respond to comments. #
Total comments: 15
Patch Set 4 : Remove unnecessary styles and reply to comments #Patch Set 5 : Remove spaces. #
Total comments: 12
Patch Set 6 : Respond to nits. #
Messages
Total messages: 35 (9 generated)
orenb@chromium.org changed reviewers: + jhawkins@chromium.org
jlklein@chromium.org changed reviewers: + jlklein@chromium.org
https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css (right): https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css:10: color: rgb(0, 150, 136); Can we share the color defs somewhere? https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js (right): https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:14: * <paper-checkbox></paper-checkbox> cr-checkbox https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:19: Polymer({ Should we also expose the toggle function?
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html (right): https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html:10: <paper-checkbox id="button" label="{{ label }}" https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js (right): https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:7: * `paper-checkbox` is a button that can be either checked or unchecked. User cr-checkbox, cr-toggle-button, etc
https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css (right): https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css:10: color: rgb(0, 150, 136); On 2015/02/11 01:55:36, Jeremy Klein wrote: > Can we share the color defs somewhere? Not sure! Any ideas, Michael or Steven? Note that this styling may change once I update the element to 0.5.4 in a separate cl which I intend to land before this one. https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html (right): https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html:10: <paper-checkbox id="button" On 2015/02/11 02:08:10, michaelpg wrote: > label="{{ label }}" Done. https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js (right): https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:7: * `paper-checkbox` is a button that can be either checked or unchecked. User On 2015/02/11 02:08:10, michaelpg wrote: > cr-checkbox, cr-toggle-button, etc Done. https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:14: * <paper-checkbox></paper-checkbox> On 2015/02/11 01:55:36, Jeremy Klein wrote: > cr-checkbox Done. https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:19: Polymer({ On 2015/02/11 01:55:36, Jeremy Klein wrote: > Should we also expose the toggle function? Done.
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css (right): https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css:10: color: rgb(0, 150, 136); On 2015/02/11 20:12:42, Oren Blasberg wrote: > On 2015/02/11 01:55:36, Jeremy Klein wrote: > > Can we share the color defs somewhere? > > Not sure! Any ideas, Michael or Steven? > > Note that this styling may change once I update the element to 0.5.4 in a > separate cl which I intend to land before this one. I agree that it would be nice to have something like a cr_elements.css file that all cr_elements share. I have no idea what the best way to implement that would be though, my CSS fu is pretty weak.
https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css (right): https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css:10: color: rgb(0, 150, 136); On 2015/02/11 20:12:42, Oren Blasberg wrote: > On 2015/02/11 01:55:36, Jeremy Klein wrote: > > Can we share the color defs somewhere? > > Not sure! Any ideas, Michael or Steven? > > Note that this styling may change once I update the element to 0.5.4 in a > separate cl which I intend to land before this one. Yes! I just discovered <core-style> last night. We should come up with a good general-purpose solution using it and then use that for our styling and theming. It has the added advantage of letting other Web UI that use cr-elements easily define their own themes. Here's a simple example: http://pastebin.com/sP2iNMng But we may want to take it a bit further, i.e., define our own <core-style id="cr-themes"> where we specify a bunch of background and foreground colors. Then we can just use those values in cr-checkbox, cr-toggle-button, etc. instead of defining the colors on each individual control. Since we'd still like to have CSS in .css files, maybe we also wrap <core-style> in a <cr-style> element which is capable of importing .css files and applying the templating. Outside the scope of this CL, but take a look and let me know what you think.
On 2015/02/11 20:40:34, michaelpg wrote: > https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... > File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css (right): > > https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... > ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css:10: color: rgb(0, > 150, 136); > On 2015/02/11 20:12:42, Oren Blasberg wrote: > > On 2015/02/11 01:55:36, Jeremy Klein wrote: > > > Can we share the color defs somewhere? > > > > Not sure! Any ideas, Michael or Steven? > > > > Note that this styling may change once I update the element to 0.5.4 in a > > separate cl which I intend to land before this one. > > Yes! I just discovered <core-style> last night. We should come up with a good > general-purpose solution using it and then use that for our styling and theming. > It has the added advantage of letting other Web UI that use cr-elements easily > define their own themes. > > Here's a simple example: > http://pastebin.com/sP2iNMng > > But we may want to take it a bit further, i.e., define our own <core-style > id="cr-themes"> where we specify a bunch of background and foreground colors. > Then we can just use those values in cr-checkbox, cr-toggle-button, etc. instead > of defining the colors on each individual control. > > Since we'd still like to have CSS in .css files, maybe we also wrap <core-style> > in a <cr-style> element which is capable of importing .css files and applying > the templating. > > Outside the scope of this CL, but take a look and let me know what you think. Great call! I totally forgot about core-style.
orenb@chromium.org changed reviewers: + dbeam@chromium.org
Good idea, thanks guys, we can follow up with the core-style changes in later CL(s)
Friendly ping on the review
lgtm
On 2015/02/13 21:25:16, Jeremy Klein wrote: > lgtm My LG is assuming a followup to update the styles after the new checkbox version lands.
https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css (right): https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css:9: paper-checkbox::shadow #ink[checked] { #ink == text/borders? https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html (right): https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html:4: href="chrome://resources/cr_elements/cr_events/cr_events.html"> that's weird. why are we using _ when polymer uses -? https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html:4: href="chrome://resources/cr_elements/cr_events/cr_events.html"> chrome cr cr cr :( https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js (right): https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:7: * `cr-checkbox` is a button that can be either checked or unchecked. User nit: \s\s -> \s https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:26: * @default false ^ what syntax is this? https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:32: why so spacious?
https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html (right): https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html:4: href="chrome://resources/cr_elements/cr_events/cr_events.html"> On 2015/02/13 21:56:49, Dan Beam wrote: > chrome cr cr cr :( cf. paper-checkbox/paper-checkbox.html "cr_events/cr_events.*" is pretty much *the* convention in Polymer-land (except the underscore). "chrome://elements/" doesn't imply "chrome elements" any more than "chrome://polymer/" implies "chrome polymer elements". https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html:4: href="chrome://resources/cr_elements/cr_events/cr_events.html"> On 2015/02/13 21:56:49, Dan Beam wrote: > that's weird. why are we using _ when polymer uses -? yeah. We decided to be consistent with the rest of Chromium and use underscores everywhere. Having hyphens in the file names but underscores in the directory names is also weird. https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js (right): https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:7: * `cr-checkbox` is a button that can be either checked or unchecked. User On 2015/02/13 21:56:50, Dan Beam wrote: > nit: \s\s -> \s I'd encourage this to be more than a nit, as double-spacing after a period represents less than 10% of our JS comments. .\s .\s\s third_party/polymer/components: 1162 99 chrome: 57304 4503 Source (imperfect): find . -type f -name "*.js" -o -name "*.html" | xargs sed "s/\.\s/\. \n/g" | grep -c "\.\s$" minus find . -type f -name "*.js" -o -name "*.html" | xargs sed "s/\.\s\s/\. \n/g" | grep -c "\.\s\s$"
https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js (right): https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:26: * @default false On 2015/02/13 21:56:50, Dan Beam wrote: > ^ what syntax is this? Polymer-flavored jsdoc: https://www.polymer-project.org/docs/start/reusableelements.html#publishing-a... and https://github.com/Polymer/context-free-parser/blob/master/context-free-parse... We haven't 100% decided what if any of these attributes we'll use, but these are used by Polymer's core-doc-viewer to generate doc pages for its elements, and Polymer suggests that others do the same. example: http://polymerlabs.github.io/seed-element/components/seed-element/ Note that the Google Web Components style guide (which is intended for public-facing Google components, so not 100% relevant in Chromium) mandates @attribute, @property, @method, @event, and @default and @required when applicable: https://github.com/GoogleWebComponents/style-guide
New patchsets have been uploaded after l-g-t-m from jlklein@chromium.org
https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css (right): https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.css:9: paper-checkbox::shadow #ink[checked] { On 2015/02/13 21:56:49, Dan Beam wrote: > #ink == text/borders? I ended up removing this because now that I've updated the element to 0.5.4, the default colors are already correct (teal) https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js (right): https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:7: * `cr-checkbox` is a button that can be either checked or unchecked. User On 2015/02/13 23:27:57, michaelpg wrote: > On 2015/02/13 21:56:50, Dan Beam wrote: > > nit: \s\s -> \s > > I'd encourage this to be more than a nit, as double-spacing after a period > represents less than 10% of our JS comments. > > .\s .\s\s > third_party/polymer/components: 1162 99 > chrome: 57304 4503 > > Source (imperfect): > find . -type f -name "*.js" -o -name "*.html" | xargs sed "s/\.\s/\. \n/g" | > grep -c "\.\s$" > minus > find . -type f -name "*.js" -o -name "*.html" | xargs sed "s/\.\s\s/\. > \n/g" | grep -c "\.\s\s$" Done. https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:32: On 2015/02/13 21:56:50, Dan Beam wrote: > why so spacious? Done.
lgtm https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html (right): https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html:10: <paper-checkbox id="button" On 2015/02/11 20:12:42, Oren Blasberg wrote: > On 2015/02/11 02:08:10, michaelpg wrote: > > label="{{ label }}" > > Done. Sorry, I've realized this is just a convention from an older project but isn't followed in Chrome or Polymer. Let's be consistent by avoiding spaces: {{label}} from here on out.
New patchsets have been uploaded after l-g-t-m from michaelpg@chromium.org
https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html (right): https://codereview.chromium.org/909313003/diff/20001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html:10: <paper-checkbox id="button" On 2015/02/18 00:11:16, michaelpg wrote: > On 2015/02/11 20:12:42, Oren Blasberg wrote: > > On 2015/02/11 02:08:10, michaelpg wrote: > > > label="{{ label }}" > > > > Done. > > Sorry, I've realized this is just a convention from an older project but isn't > followed in Chrome or Polymer. Let's be consistent by avoiding spaces: {{label}} > from here on out. Done.
https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html (right): https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html:4: href="chrome://resources/cr_elements/cr_events/cr_events.html"> On 2015/02/13 23:27:57, michaelpg wrote: > On 2015/02/13 21:56:49, Dan Beam wrote: > > chrome cr cr cr :( > > cf. paper-checkbox/paper-checkbox.html that's repetitive. that's repetitive. it wastes space and my time. it wastes space and my time. don't you wish you got those 10s back? don't you wish you got those 10s back? > > "cr_events/cr_events.*" is pretty much *the* convention in Polymer-land (except > the underscore). I'm confused: are we in chrome or polymer land? you just said "we willingly disobey polymer when it comes to - vs _" but we don't when it comes to saving us tons of wrapped lines? > > "chrome://elements/" doesn't imply "chrome elements" any more than > "chrome://polymer/" implies "chrome polymer elements". I don't get what you mean here. English is made up of words strung together. Code is written in/like English. That's why conditional operators are if/else/then, for example, so it reads like logical English. When you glean the English of "chrome://elements", YES is does imply "chrome elements" and "chrome://polymer" does imply "using polymer in Chrome". If we're against writing a new data source for this that's OK I guess, but if the only way these things will ever be loaded is via a URL that starts with chrome:// it seems pretty silly to add 3 more "cr" to it... https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js (right): https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:17: * @element cr-checkbox are we writing chrome JS or Polymer? does this get ignored when compiled? https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:30: reflect: true nit: , (we don't care about IE and this lessens diffs later and lets you sort keys more easily) https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:51: reflect: true nit: if we're gonna copy this {value: false, relect: true} literal a lot, maybe make a constant somewhere https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:55: toggle: function() { nit: doc comment https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:58: same
https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js (right): https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:17: * @element cr-checkbox On 2015/02/19 22:47:56, Dan Beam wrote: > are we writing chrome JS or Polymer? does this get ignored when compiled? Yes this will be ignored when compiled. It is here in case we want to generate the documentation sites like the other Polymer elements have. Same goes for "@attribute, @default, @event, etc. https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:51: reflect: true On 2015/02/19 22:47:56, Dan Beam wrote: > nit: if we're gonna copy this {value: false, reflect: true} literal a lot, maybe > make a constant somewhere I don't expect that we'll copy this a whole lot so I think it's useful to be explicit. I think it might be just as common to have {value: true, reflect: true}. Maybe we can extract both to constants, but I don't really think the win is too big. By the way, is it OK from a style perspective to inline these objects?
ultimately lgtm, the naming issue can be resolved separately https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js (right): https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:51: reflect: true On 2015/02/19 22:58:00, Jeremy Klein wrote: > On 2015/02/19 22:47:56, Dan Beam wrote: > > nit: if we're gonna copy this {value: false, reflect: true} literal a lot, > maybe > > make a constant somewhere > > I don't expect that we'll copy this a whole lot so I think it's useful to be > explicit. I think it might be just as common to have {value: true, reflect: > true}. Maybe we can extract both to constants, but I don't really think the win > is too big. > > By the way, is it OK from a style perspective to inline these objects? yes
New patchsets have been uploaded after l-g-t-m from dbeam@chromium.org
https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js (right): https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:30: reflect: true On 2015/02/19 22:47:56, Dan Beam wrote: > nit: , (we don't care about IE and this lessens diffs later and lets you sort > keys more easily) Ended up putting on one line. https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:51: reflect: true On 2015/02/19 22:58:00, Jeremy Klein wrote: > On 2015/02/19 22:47:56, Dan Beam wrote: > > nit: if we're gonna copy this {value: false, reflect: true} literal a lot, > maybe > > make a constant somewhere > > I don't expect that we'll copy this a whole lot so I think it's useful to be > explicit. I think it might be just as common to have {value: true, reflect: > true}. Maybe we can extract both to constants, but I don't really think the win > is too big. > > By the way, is it OK from a style perspective to inline these objects? Done. https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:55: toggle: function() { On 2015/02/19 22:47:56, Dan Beam wrote: > nit: doc comment Done. https://codereview.chromium.org/909313003/diff/80001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.js:58: On 2015/02/19 22:47:56, Dan Beam wrote: > same Done.
The CQ bit was checked by orenb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/909313003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1493ca1c409b970874c69095e89b39edbfae2882 Cr-Commit-Position: refs/heads/master@{#317190}
Message was sent while issue was closed.
Anyone opposed to removing "cr_" from "chrome://cr_elements"? And if anyone has a concrete suggestion for shortening these URLs, it'd be great to get this settled: "chrome://elements/cr_checkbox/cr_checkbox.html" https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html (right): https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html:4: href="chrome://resources/cr_elements/cr_events/cr_events.html"> On 2015/02/19 22:47:56, Dan Beam wrote: > On 2015/02/13 23:27:57, michaelpg wrote: > > On 2015/02/13 21:56:49, Dan Beam wrote: > > > chrome cr cr cr :( > > > > cf. paper-checkbox/paper-checkbox.html > > that's repetitive. that's repetitive. > > it wastes space and my time. it wastes space and my time. > > don't you wish you got those 10s back? don't you wish you got those 10s back? My editor cuts this down to 2s... but I acknowledge that it's repetitive. > > > > > "cr_events/cr_events.*" is pretty much *the* convention in Polymer-land > (except > > the underscore). > > I'm confused: are we in chrome or polymer land? you just said "we willingly > disobey polymer when it comes to - vs _" but we don't when it comes to saving us > tons of wrapped lines? Both. And this is one way we reconcile that. This is the convention that has formed around web components. Just because we're enforcing Chrome style (- vs _) and making use of Chrome facilities (e.g. including with "chrome://" instead of relative paths) doesn't mean we should assume nobody will be interested in using some of our components in their projects. Also, we aren't assuming all files will have the same names as their directories. For instance, "paper-dropdown/paper-dropdown-transition.html" is used by "paper-dropdown/paper-dropdown.html" (yay composition). But it makes it clear that: paper-dropdown/paper-dropdown.html paper-dropdown/paper-dropdown-transition.html both belong to paper-dropdown, while paper-dropdown-menu/paper-dropdown-menu.html belongs to the separate paper-dropdown-menu component. I don't see a great way to encode that information while saving a substantial number of characters... we could leave out "cr_" in the leaf directories. > > > > > "chrome://elements/" doesn't imply "chrome elements" any more than > > "chrome://polymer/" implies "chrome polymer elements". > > I don't get what you mean here. > > English is made up of words strung together. Code is written in/like English. > That's why conditional operators are if/else/then, for example, so it reads like > logical English. > > When you glean the English of "chrome://elements", YES is does imply "chrome > elements" and "chrome://polymer" does imply "using polymer in Chrome". I meant that "using elements in chrome" is not the same as "using custom elements in chrome that are specific to chrome rather than a third party". If "Polymer" can be top-level, Chrome elements should go in a "Chrome" top-level "directory", thus "polymer" and "cr_elements". I don't care much though, so "chrome://elements" is fine with me. > > If we're against writing a new data source for this that's OK I guess, but if > the only way these things will ever be loaded is via a URL that starts with > chrome:// it seems pretty silly to add 3 more "cr" to it... Not sure what you mean, we added this to shared_resources_data_source rather than create a new data source, but it's easily changeable either way.
Message was sent while issue was closed.
On 2015/02/20 01:26:48, michaelpg wrote: > Anyone opposed to removing "cr_" from "chrome://cr_elements"? > > And if anyone has a concrete suggestion for shortening these URLs, it'd be great > to get this settled: > > "chrome://elements/cr_checkbox/cr_checkbox.html" > > https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... > File ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html (right): > > https://codereview.chromium.org/909313003/diff/40001/ui/webui/resources/cr_el... > ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html:4: > href="chrome://resources/cr_elements/cr_events/cr_events.html"> > On 2015/02/19 22:47:56, Dan Beam wrote: > > On 2015/02/13 23:27:57, michaelpg wrote: > > > On 2015/02/13 21:56:49, Dan Beam wrote: > > > > chrome cr cr cr :( > > > > > > cf. paper-checkbox/paper-checkbox.html > > > > that's repetitive. that's repetitive. > > > > it wastes space and my time. it wastes space and my time. > > > > don't you wish you got those 10s back? don't you wish you got those 10s back? > > My editor cuts this down to 2s... but I acknowledge that it's repetitive. > > > > > > > > > "cr_events/cr_events.*" is pretty much *the* convention in Polymer-land > > (except > > > the underscore). > > > > I'm confused: are we in chrome or polymer land? you just said "we willingly > > disobey polymer when it comes to - vs _" but we don't when it comes to saving > us > > tons of wrapped lines? > > Both. And this is one way we reconcile that. This is the convention that has > formed around web components. > > Just because we're enforcing Chrome style (- vs _) and making use of Chrome > facilities (e.g. including with "chrome://" instead of relative paths) doesn't > mean we should assume nobody will be interested in using some of our components > in their projects. > > Also, we aren't assuming all files will have the same names as their > directories. For instance, "paper-dropdown/paper-dropdown-transition.html" is > used by "paper-dropdown/paper-dropdown.html" (yay composition). But it makes it > clear that: > > paper-dropdown/paper-dropdown.html > paper-dropdown/paper-dropdown-transition.html > > both belong to paper-dropdown, while > > paper-dropdown-menu/paper-dropdown-menu.html > > belongs to the separate paper-dropdown-menu component. I don't see a great way > to encode that information while saving a substantial number of characters... we > could leave out "cr_" in the leaf directories. > > > > > > > > > "chrome://elements/" doesn't imply "chrome elements" any more than > > > "chrome://polymer/" implies "chrome polymer elements". > > > > I don't get what you mean here. > > > > English is made up of words strung together. Code is written in/like English. > > That's why conditional operators are if/else/then, for example, so it reads > like > > logical English. > > > > When you glean the English of "chrome://elements", YES is does imply "chrome > > elements" and "chrome://polymer" does imply "using polymer in Chrome". > > I meant that "using elements in chrome" is not the same as "using custom > elements in chrome that are specific to chrome rather than a third party". If > "Polymer" can be top-level, Chrome elements should go in a "Chrome" top-level > "directory", thus "polymer" and "cr_elements". > > I don't care much though, so "chrome://elements" is fine with me. > > > > > If we're against writing a new data source for this that's OK I guess, but if > > the only way these things will ever be loaded is via a URL that starts with > > chrome:// it seems pretty silly to add 3 more "cr" to it... > > Not sure what you mean, we added this to shared_resources_data_source rather > than create a new data source, but it's easily changeable either way. Definite +1 to chrome://elements/cr_checkbox/cr_checkbox.html I'd actually like to take this a step further and serve *all* elements from that directory so that our html imports can all be simple ../blah/blah.html just like with the paper elements. This doesn't mean that the elements need to be in the same place in the source tree, but I do think simplifying the url structure would be beneficial. Splitting up polymer and cr elements doesn't seem like a huge win to me when we need to namespace the tag names anyway. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
