Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1331573003:
MD Downloads: compute card elevation via data binding (Closed)
DescriptionMD Downloads: compute card elevation via data binding
R=tommycli@chromium.org
BUG=526577
Committed: https://crrev.com/ef07bc4910dedd766f075b625bc4e158bde5b2ac
Cr-Commit-Position: refs/heads/master@{#348003}
Patch Set 1 #Patch Set 2 : whoops #
Total comments: 8
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 13 (1 generated)
lgtm sans comment #1 below https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_downloads/item.html (right): https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... chrome/browser/resources/md_downloads/item.html:29: Can't have any line breaks. These line breaks are okay right? It's <span><!-- no line breaks here -->[[binding]]<!-- no line breaks here --></span> that's not ok right? https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_downloads/item.js (right): https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... chrome/browser/resources/md_downloads/item.js:189: computeElevation_: function() { I still feel uneasy about this. But I talked to michaelpg and he agrees with you. This is not a blocker, just a complaint.
https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_downloads/item.html (right): https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... chrome/browser/resources/md_downloads/item.html:29: Can't have any line breaks. On 2015/09/09 20:49:16, tommycli wrote: > These line breaks are okay right? > > It's <span><!-- no line breaks here -->[[binding]]<!-- no line breaks here > --></span> that's not ok right? yes, <!-- \n --> is OK https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_downloads/item.js (right): https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... chrome/browser/resources/md_downloads/item.js:189: computeElevation_: function() { On 2015/09/09 20:49:16, tommycli wrote: > I still feel uneasy about this. > > But I talked to michaelpg and he agrees with you. This is not a blocker, just a > complaint. Acknowledged.
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1331573003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1331573003/20001
https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_downloads/item.html (right): https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... chrome/browser/resources/md_downloads/item.html:29: Can't have any line breaks. On 2015/09/09 21:00:15, Dan Beam wrote: > On 2015/09/09 20:49:16, tommycli wrote: > > These line breaks are okay right? > > > > It's <span><!-- no line breaks here -->[[binding]]<!-- no line breaks here > > --></span> that's not ok right? > > yes, <!-- \n --> is OK I mean, you don't need that comment to span the whitespace right? <div id="title-area"> <a is="action-link"...> is okay right?
https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_downloads/item.html (right): https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... chrome/browser/resources/md_downloads/item.html:29: Can't have any line breaks. On 2015/09/09 21:03:03, tommycli wrote: > On 2015/09/09 21:00:15, Dan Beam wrote: > > On 2015/09/09 20:49:16, tommycli wrote: > > > These line breaks are okay right? > > > > > > It's <span><!-- no line breaks here -->[[binding]]<!-- no line breaks here > > > --></span> that's not ok right? > > > > yes, <!-- \n --> is OK > > I mean, you don't need that comment to span the whitespace right? > > <div id="title-area"> > <a is="action-link"...> > > is okay right? no, <div id="title-area"><!-- doesn't matter what happens here! I'm in a comment! --><a is="action-link"> is OK
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ef07bc4910dedd766f075b625bc4e158bde5b2ac Cr-Commit-Position: refs/heads/master@{#348003}
Message was sent while issue was closed.
https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_downloads/item.html (right): https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... chrome/browser/resources/md_downloads/item.html:29: Can't have any line breaks. On 2015/09/09 21:04:10, Dan Beam wrote: > On 2015/09/09 21:03:03, tommycli wrote: > > On 2015/09/09 21:00:15, Dan Beam wrote: > > > On 2015/09/09 20:49:16, tommycli wrote: > > > > These line breaks are okay right? > > > > > > > > It's <span><!-- no line breaks here -->[[binding]]<!-- no line breaks here > > > > --></span> that's not ok right? > > > > > > yes, <!-- \n --> is OK > > > > I mean, you don't need that comment to span the whitespace right? > > > > <div id="title-area"> > > <a is="action-link"...> > > > > is okay right? > > no, <div id="title-area"><!-- > > doesn't matter what happens here! > I'm in a comment! > > --><a is="action-link"> > > is OK Ah, I think we're mis-communicating. I mean: why are those two HTML comments necessary?
Message was sent while issue was closed.
https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... File chrome/browser/resources/md_downloads/item.html (right): https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... chrome/browser/resources/md_downloads/item.html:29: Can't have any line breaks. On 2015/09/09 21:10:44, tommycli wrote: > On 2015/09/09 21:04:10, Dan Beam wrote: > > On 2015/09/09 21:03:03, tommycli wrote: > > > On 2015/09/09 21:00:15, Dan Beam wrote: > > > > On 2015/09/09 20:49:16, tommycli wrote: > > > > > These line breaks are okay right? > > > > > > > > > > It's <span><!-- no line breaks here -->[[binding]]<!-- no line breaks > here > > > > > --></span> that's not ok right? > > > > > > > > yes, <!-- \n --> is OK > > > > > > I mean, you don't need that comment to span the whitespace right? > > > > > > <div id="title-area"> > > > <a is="action-link"...> > > > > > > is okay right? > > > > no, <div id="title-area"><!-- > > > > doesn't matter what happens here! > > I'm in a comment! > > > > --><a is="action-link"> > > > > is OK > > Ah, I think we're mis-communicating. > > I mean: why are those two HTML comments necessary? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... https://developer.mozilla.org/en-US/docs/Web/CSS/word-break and btw, i don't believe <div><!-- this is ok -->[[computeThing_(param)]]<!-- because [[]] needs to be the whole textContent --></div>
Message was sent while issue was closed.
Ah I get it now. Thanks for the explanation. On Wed, Sep 9, 2015 at 2:33 PM, <dbeam@chromium.org> wrote: > > > https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... > File chrome/browser/resources/md_downloads/item.html (right): > > > https://codereview.chromium.org/1331573003/diff/20001/chrome/browser/resource... > chrome/browser/resources/md_downloads/item.html:29: Can't have any line > breaks. > On 2015/09/09 21:10:44, tommycli wrote: > >> On 2015/09/09 21:04:10, Dan Beam wrote: >> > On 2015/09/09 21:03:03, tommycli wrote: >> > > On 2015/09/09 21:00:15, Dan Beam wrote: >> > > > On 2015/09/09 20:49:16, tommycli wrote: >> > > > > These line breaks are okay right? >> > > > > >> > > > > It's <span><!-- no line breaks here -->[[binding]]<!-- no line >> > breaks > >> here >> > > > > --></span> that's not ok right? >> > > > >> > > > yes, <!-- \n --> is OK >> > > >> > > I mean, you don't need that comment to span the whitespace right? >> > > >> > > <div id="title-area"> >> > > <a is="action-link"...> >> > > >> > > is okay right? >> > >> > no, <div id="title-area"><!-- >> > >> > doesn't matter what happens here! >> > I'm in a comment! >> > >> > --><a is="action-link"> >> > >> > is OK >> > > Ah, I think we're mis-communicating. >> > > I mean: why are those two HTML comments necessary? >> > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > https://developer.mozilla.org/en-US/docs/Web/CSS/word-break > > and btw, i don't believe <div><!-- this is ok > -->[[computeThing_(param)]]<!-- because [[]] needs to be the whole > textContent --></div> > > https://codereview.chromium.org/1331573003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ef07bc4910dedd766f075b625bc4e158bde5b2ac Cr-Commit-Position: refs/heads/master@{#348003} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
