Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1023673008:
Add Media Router issue-banner. (Closed)
DescriptionAdd Media Router issue-banner.
This custom polymer element is used to show issues related to casting. It is styled after the current cast extension's issue banner.
BUG=464222
Committed: https://crrev.com/7e0be9b510179367a120696203d0f7ff6adec06c
Cr-Commit-Position: refs/heads/master@{#323991}
Patch Set 1 : #
Total comments: 9
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 12
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 30 (11 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
apacible@chromium.org changed reviewers: + jhawkins@chromium.org, jlklein@chromium.org, mfoltz@chromium.org, michaelpg@chromium.org
PTAL, thanks! +jhawkins: browser_resources.grd (owner) & general feedback +michaelpg, jlklein: polymer feedback +mfoltz: media router (owner)
https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html (right): https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html:2: <link rel="stylesheet" href="issue_banner_dir.css"> I think you can get rid of this now because of the host_context solution. https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html:6: <template bind="{{issue}}"> I'm confused why you need this and how things like "onClickAction" are working if this is being bound to the issue. https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js (right): https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:14: issue: undefined nit: The convention I've seen for this has been to use null instead of undefined. I'm not entirely sure why, but it seems like we should keep that for consistency unless you have a specific reason to use undefined. https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:18: this.issue = undefined; You don't need to do this unless you're actually initializing this to an object. https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:21: onClickAction: function(event, detail, sender) { nit: JSDocs
https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html (right): https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html:6: <template bind="{{issue}}"> I'm confused why you need this and how things like "onClickAction" are working if this is being bound to the issue. https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js (right): https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:14: issue: undefined nit: The convention I've seen for this has been to use null instead of undefined. I'm not entirely sure why, but it seems like we should keep that for consistency unless you have a specific reason to use undefined. https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:18: this.issue = undefined; You don't need to do this unless you're actually initializing this to an object. https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:21: onClickAction: function(event, detail, sender) { nit: JSDocs
Patchset #2 (id:160001) has been deleted
https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html (right): https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html:2: <link rel="stylesheet" href="issue_banner_dir.css"> On 2015/04/06 18:49:57, Jeremy Klein wrote: > I think you can get rid of this now because of the host_context solution. Done. https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html:6: <template bind="{{issue}}"> On 2015/04/06 18:49:57, Jeremy Klein wrote: > I'm confused why you need this and how things like "onClickAction" are working > if this is being bound to the issue. Talked offline. Removed this line and added issue.* where appropriate. Interestingly, "onClickAction" was working while issue was binded. https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js (right): https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:14: issue: undefined On 2015/04/06 18:49:58, Jeremy Klein wrote: > nit: The convention I've seen for this has been to use null instead of > undefined. I'm not entirely sure why, but it seems like we should keep that for > consistency unless you have a specific reason to use undefined. Done. https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:18: this.issue = undefined; On 2015/04/06 18:49:57, Jeremy Klein wrote: > You don't need to do this unless you're actually initializing this to an object. Done. https://codereview.chromium.org/1023673008/diff/140001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:21: onClickAction: function(event, detail, sender) { On 2015/04/06 18:49:58, Jeremy Klein wrote: > nit: JSDocs Added.
lgtm With nits. https://codereview.chromium.org/1023673008/diff/180001/chrome/browser/browser... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/1023673008/diff/180001/chrome/browser/browser... chrome/browser/browser_resources.grd:456: <include name="IDR_ISSUE_BANNER_HTML" file="resources\media_router\elements\issue_banner\issue_banner.html" type="BINDATA" /> note: I'd recommend pulling your resources out into their own grdp and then just including that here because this file's huge. You can do that in its own CL if you'd like. https://codereview.chromium.org/1023673008/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js (right): https://codereview.chromium.org/1023673008/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:17: created: function() { You can remove this whole function. https://codereview.chromium.org/1023673008/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:25: * @this {issue-banner} Do you need this?
https://codereview.chromium.org/1023673008/diff/180001/chrome/browser/browser... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/1023673008/diff/180001/chrome/browser/browser... chrome/browser/browser_resources.grd:456: <include name="IDR_ISSUE_BANNER_HTML" file="resources\media_router\elements\issue_banner\issue_banner.html" type="BINDATA" /> On 2015/04/06 21:07:32, Jeremy Klein wrote: > note: I'd recommend pulling your resources out into their own grdp and then just > including that here because this file's huge. You can do that in its own CL if > you'd like. Sounds good; I'll do that in a separate CL. https://codereview.chromium.org/1023673008/diff/180001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js (right): https://codereview.chromium.org/1023673008/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:17: created: function() { On 2015/04/06 21:07:32, Jeremy Klein wrote: > You can remove this whole function. Done. https://codereview.chromium.org/1023673008/diff/180001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:25: * @this {issue-banner} On 2015/04/06 21:07:32, Jeremy Klein wrote: > Do you need this? Removed.
Can you upload a screenshot?
https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css (right): https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css:40: display: block; Why is this necessary? https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css:41: line-height: 18px; Hmm, I'm thinking this should probably be expressed in em, as this won't scale as the user modifies font size. https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css:54: padding-left: 8px; You're gonna kill me: we can just use -webkit-padding-start: 8px (which automatically handles direction appropriately). https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html (right): https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html:29: <br/> Don't use <br>. Adjust margins instead (although the fact that you're using a span for the text above makes me think you just want a line break here, so use <div> instead of <span>). https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html:37: {{issue.defaultActionText}}</span> nit: Move </span> to a new line. https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js (right): https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:22: * @param {Object} detail The details of the event. Can you use a well-typed typedef instead of Object?
Screenshots (with the below changes): LTR Blocking: https://screenshot.googleplex.com/TZbfBg4MRC RTL Blocking: https://screenshot.googleplex.com/UWYTYMJ0oD LTR Non-Blocking: https://screenshot.googleplex.com/y6bP3EFmMS RTL Non-Blocking: https://screenshot.googleplex.com/K17vbQCqpt https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css (right): https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css:40: display: block; On 2015/04/06 21:52:47, James Hawkins wrote: > Why is this necessary? Removed; changed <span> tags to <div>. https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css:41: line-height: 18px; On 2015/04/06 21:52:47, James Hawkins wrote: > Hmm, I'm thinking this should probably be expressed in em, as this won't scale > as the user modifies font size. Updated all line-heights to em. https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css:54: padding-left: 8px; On 2015/04/06 21:52:47, James Hawkins wrote: > You're gonna kill me: we can just use -webkit-padding-start: 8px (which > automatically handles direction appropriately). Fixed. https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html (right): https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html:29: <br/> On 2015/04/06 21:52:47, James Hawkins wrote: > Don't use <br>. Adjust margins instead (although the fact that you're using a > span for the text above makes me think you just want a line break here, so use > <div> instead of <span>). Done. Also updated above <spans> to <div> so I don't need to use display: block; on the css. https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.html:37: {{issue.defaultActionText}}</span> On 2015/04/06 21:52:48, James Hawkins wrote: > nit: Move </span> to a new line. Done. Moved above one as well. https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js (right): https://codereview.chromium.org/1023673008/diff/200001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.js:22: * @param {Object} detail The details of the event. On 2015/04/06 21:52:48, James Hawkins wrote: > Can you use a well-typed typedef instead of Object? I talked to jlklein@ about these @params; detail is denoted as 'Object' because it isn't typed well for these events. While we can check its contents and be more specific, we don't use detail here at all so he suggests we just keep this as 'Object'.
Learn more link is missing on 4.png (RTL)
On 2015/04/06 22:54:06, James Hawkins wrote: > Learn more link is missing on 4.png (RTL) Here's the RTL Non-Blocking that matches LTR: https://screenshot.googleplex.com/DmhHAJgAj3.png 4.png would be what it looks like with only the primary action RTL.
https://codereview.chromium.org/1023673008/diff/220001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css (right): https://codereview.chromium.org/1023673008/diff/220001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css:47: display: block; This needs to be block as well? https://codereview.chromium.org/1023673008/diff/220001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css:56: display: block; This needs to be block as well?
https://codereview.chromium.org/1023673008/diff/220001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css (right): https://codereview.chromium.org/1023673008/diff/220001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css:47: display: block; On 2015/04/06 23:09:22, James Hawkins wrote: > This needs to be block as well? Removed. https://codereview.chromium.org/1023673008/diff/220001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css:56: display: block; On 2015/04/06 23:09:22, James Hawkins wrote: > This needs to be block as well? Removed.
lgtm
lgtm https://codereview.chromium.org/1023673008/diff/240001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css (right): https://codereview.chromium.org/1023673008/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css:6: -webkit-appearance: none; none is default. remove?
https://codereview.chromium.org/1023673008/diff/240001/chrome/browser/resourc... File chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css (right): https://codereview.chromium.org/1023673008/diff/240001/chrome/browser/resourc... chrome/browser/resources/media_router/elements/issue_banner/issue_banner.css:6: -webkit-appearance: none; On 2015/04/06 23:23:59, mark a. foltz wrote: > none is default. remove? Done.
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jlklein@chromium.org, mfoltz@chromium.org, jhawkins@chromium.org Link to the patchset: https://codereview.chromium.org/1023673008/#ps260001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1023673008/260001
Message was sent while issue was closed.
Committed patchset #6 (id:260001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7e0be9b510179367a120696203d0f7ff6adec06c Cr-Commit-Position: refs/heads/master@{#323991} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
