Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2950673002:
[MD Settings][Night Light] CL8: Add the ripple focus effect to the slider knobs (Closed)
|
Created:
3 years, 6 months ago by afakhry Modified:
3 years, 6 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org, vitalyp+closure_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MD Settings][Night Light] CL8: Add the ripple focus effect to the slider knobs
Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c20
BUG=705816
Review-Url: https://codereview.chromium.org/2950673002
Cr-Commit-Position: refs/heads/master@{#481035}
Committed: https://chromium.googlesource.com/chromium/src/+/151428415415bf5f90834e350fc53004668a9c8e
Patch Set 1 #
Total comments: 40
Patch Set 2 : stevenjb's and dpapad's comments #Patch Set 3 : no I18nBehavior #Patch Set 4 : Rebase on localization CL. #Patch Set 5 : dpapad's comments #
Total comments: 2
Patch Set 6 : Nit #
Messages
Total messages: 35 (19 generated)
Description was changed from ========== [Night Light] CL8: Add the ripple focus effect to the slider knobs Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c20 BUG=705816 ========== to ========== [Night Light] CL8: Add the ripple focus effect to the slider knobs Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c20 BUG=705816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
afakhry@chromium.org changed reviewers: + stevenjb@chromium.org
Steven, can you please take a look? Thank you!
stevenjb@chromium.org changed reviewers: + dpapad@chromium.org
+dpapad@ whose DOM fu is much better than mine. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/night_light_slider.html (right): https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.html:136: color: rgb(51, 103, 214); Use var(--google-blue-700) throughout this file https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.html:175: <div id="hidden" hidden></div> This is a very non descriptive id; what is it for? https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:470: * Checks if any of the two knobs is focused. s/any/either/ (Also, redundant comment not needed) https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:473: * otherwise. s/false otherwise// https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:485: * @private @override https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:493: // function return a ripple element. So to avoid crashes, we'll setup the returns https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:495: this._rippleContainer = this.$.hidden; Ugh, this is kind of awkward. Can we just create a dummy unparented element? +dpapad@ who might have a suggestion. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:525: /** @private */ @override; also, group the overrides together.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I don't have enough information to provide useful feedback here. None of the video attachments in the bug loading for me. Can you provide screenshots/screencasts? My initial question is why do we need to implement such low level logic? I don't see any other element in MD Settings to implement PaperInkyFocusBehavior. Doesn't paper-ripple already do what we want, and could it be used instead? What exactly is night_light_slider? https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:76: this.$.startKnob.addEventListener('focus', this.onFocus_.bind(this)); Why are those listeners added programmatically instead of declaratively? https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:126: this.dragObject_ = this.$.startKnob; Is dragObject_ used in the HTML? If not, there is no good reason to have it as a Polymer property. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:473: * otherwise. On 2017/06/20 at 00:31:44, stevenjb wrote: > s/false otherwise// Or even shorter as follows: "Whether either of the two knobs is focused."
On 2017/06/20 01:24:47, dpapad wrote: > I don't have enough information to provide useful feedback here. None of the > video attachments in the bug loading for me. Can you provide > screenshots/screencasts? > > My initial question is why do we need to implement such low level logic? I don't > see any other element in MD Settings to implement PaperInkyFocusBehavior. > Doesn't paper-ripple already do what we want, and could it be used instead? > > What exactly is night_light_slider? > > https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/device_page/night_light_slider.js > (right): > > https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/device_page/night_light_slider.js:76: > this.$.startKnob.addEventListener('focus', this.onFocus_.bind(this)); > Why are those listeners added programmatically instead of declaratively? > > https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/device_page/night_light_slider.js:126: > this.dragObject_ = this.$.startKnob; > Is dragObject_ used in the HTML? If not, there is no good reason to have it as a > Polymer property. > > https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/device_page/night_light_slider.js:473: * > otherwise. > On 2017/06/20 at 00:31:44, stevenjb wrote: > > s/false otherwise// > > Or even shorter as follows: > > "Whether either of the two knobs is focused." This is a custom slider for the nightlight feature. It has a separate start point and end point and has gone through at least preliminary UX review. The last video in the CL (comment #20) works fine for me, maybe try playing it on a chromebook? :) I was mostly looking for any suggestions on a better way to create a 'dummy element' to hide the ripple on the unfocused knob (or an alternative suggestion). If your plate is full I can ask michaelpg@ to take a look also.
>I was mostly looking for any suggestions on a better way to create a 'dummy element' to hide the ripple on the unfocused knob (or an alternative suggestion). >If your plate is full I can ask michaelpg@ to take a look also. Sorry, don't have a good suggestion for the ripple, since I am not that familiar with it (or the involved behaviors). Left some drive-by comments though. Feel free to replace me with @michaelp in the reviewers list. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/night_light_slider.html (right): https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.html:168: <!-- TODO(afakhry): Check if these values need to be localized. --> Yes, they do. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:12: /** @const */ var HOURS_PER_DAY = 24; These are on global scope. Can you wrap the contents of this file in an function scope to avoid this? See example at https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people.... https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:21: I18nBehavior, Is this used anywhere? https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:42: startTime_: { Nit: Use shorthand (here and elsewhere, where applicable) startTime_: String, https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:65: ready: function() { Missing annotation: @override https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:495: this._rippleContainer = this.$.hidden; On 2017/06/20 at 00:31:44, stevenjb wrote: > Ugh, this is kind of awkward. Can we just create a dummy unparented element? > > +dpapad@ who might have a suggestion. Don't have a good suggestion.
https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/night_light_slider.html (right): https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.html:136: color: rgb(51, 103, 214); On 2017/06/20 00:31:44, stevenjb wrote: > Use var(--google-blue-700) throughout this file > Done. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.html:168: <!-- TODO(afakhry): Check if these values need to be localized. --> On 2017/06/20 17:35:19, dpapad wrote: > Yes, they do. Already in CQ: https://codereview.chromium.org/2951483003/ https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.html:175: <div id="hidden" hidden></div> On 2017/06/20 00:31:44, stevenjb wrote: > This is a very non descriptive id; what is it for? I changed the ID to dummyRippleContainer. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:12: /** @const */ var HOURS_PER_DAY = 24; On 2017/06/20 17:35:19, dpapad wrote: > These are on global scope. Can you wrap the contents of this file in an function > scope to avoid this? See example at > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people.... Done. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:21: I18nBehavior, On 2017/06/20 17:35:20, dpapad wrote: > Is this used anywhere? It's used in the HTML $i18n{...}. Isn't that what it's for? https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:42: startTime_: { On 2017/06/20 17:35:19, dpapad wrote: > Nit: Use shorthand (here and elsewhere, where applicable) > > startTime_: String, Done. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:65: ready: function() { On 2017/06/20 17:35:19, dpapad wrote: > Missing annotation: > > @override Done. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:76: this.$.startKnob.addEventListener('focus', this.onFocus_.bind(this)); On 2017/06/20 01:24:47, dpapad wrote: > Why are those listeners added programmatically instead of declaratively? I have no idea. I tried that at first, but Chrome kept complaining with the following error: "Refused to execute inline event handler because it violates the following Content Security Policy directive: "script-src chrome://resources 'self' 'unsafe-eval'". Either the 'unsafe-inline' keyword, a hash ('sha256-...'), or a nonce ('nonce-...') is required to enable inline execution." https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:126: this.dragObject_ = this.$.startKnob; On 2017/06/20 01:24:47, dpapad wrote: > Is dragObject_ used in the HTML? If not, there is no good reason to have it as a > Polymer property. Sorry, I'm not very familiar with javascript. What's the alternative? Should I define it somewhere in the file? Or should I just use it as is and it will be added to the prototype of object 'this'? https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:470: * Checks if any of the two knobs is focused. On 2017/06/20 00:31:44, stevenjb wrote: > s/any/either/ > (Also, redundant comment not needed) Done. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:473: * otherwise. On 2017/06/20 00:31:44, stevenjb wrote: > s/false otherwise// Done. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:473: * otherwise. On 2017/06/20 00:31:44, stevenjb wrote: > s/false otherwise// Done. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:485: * @private On 2017/06/20 00:31:44, stevenjb wrote: > @override It doesn't actually override, but rather shadows that function we get from the behavior. If I add @override, the closure compiler complains with: ## /work2/chromium/src/chrome/browser/resources/settings/device_page/night_light_slider.js:489: ERROR - property _createRipple not defined on any superclass of NightLightSliderElement ## _createRipple: function() { ## ^^^^^^^^^^^^ https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:493: // function return a ripple element. So to avoid crashes, we'll setup the On 2017/06/20 00:31:44, stevenjb wrote: > returns Done. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:495: this._rippleContainer = this.$.hidden; On 2017/06/20 00:31:44, stevenjb wrote: > Ugh, this is kind of awkward. Can we just create a dummy unparented element? > > +dpapad@ who might have a suggestion. If the ripple container is null, the behavior will use the root: https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... Which will show a huge circle in the middle of the page! The only problem here that makes it different from other polymer elements is that we have two different ripple container per the same polymer element and they should change (e.g. from the start knob to the end knob or vice versa) while focus is still in the same parent polymer element. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:525: /** @private */ On 2017/06/20 00:31:44, stevenjb wrote: > @override; also, group the overrides together. This is also not an override, but shadowing of the Behavior's method with the same name. See my comment above. Maybe change the comment and remove the word "override" to avoid confusion?
https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:21: I18nBehavior, On 2017/06/20 19:59:54, afakhry wrote: > On 2017/06/20 17:35:20, dpapad wrote: > > Is this used anywhere? > > It's used in the HTML $i18n{...}. Isn't that what it's for? No, $i18n is a pre-processing macro. I18nBehavior is for i18n() calls in the JS.
https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:21: I18nBehavior, On 2017/06/20 20:19:02, stevenjb wrote: > On 2017/06/20 19:59:54, afakhry wrote: > > On 2017/06/20 17:35:20, dpapad wrote: > > > Is this used anywhere? > > > > It's used in the HTML $i18n{...}. Isn't that what it's for? > > No, $i18n is a pre-processing macro. I18nBehavior is for i18n() calls in the JS. Oh, I see. In that case, removed.
https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:76: this.$.startKnob.addEventListener('focus', this.onFocus_.bind(this)); On 2017/06/20 at 19:59:54, afakhry wrote: > On 2017/06/20 01:24:47, dpapad wrote: > > Why are those listeners added programmatically instead of declaratively? > > I have no idea. I tried that at first, but Chrome kept complaining with the following error: > > "Refused to execute inline event handler because it violates the following Content Security Policy directive: "script-src chrome://resources 'self' 'unsafe-eval'". Either the 'unsafe-inline' keyword, a hash ('sha256-...'), or a nonce ('nonce-...') is required to enable inline execution." That means that the declarative syntax was most likely incorrect. It should look roughly like this <button on-tap="onTap_">....</button> where onTap_ is a method you define in the JS. See examples https://cs.chromium.org/search/?q=%22on-tap%22+file:%5Esrc/chrome/browser/res.... https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:126: this.dragObject_ = this.$.startKnob; On 2017/06/20 at 19:59:54, afakhry wrote: > On 2017/06/20 01:24:47, dpapad wrote: > > Is dragObject_ used in the HTML? If not, there is no good reason to have it as a > > Polymer property. > > Sorry, I'm not very familiar with javascript. What's the alternative? Should I define it somewhere in the file? Or should I just use it as is and it will be added to the prototype of object 'this'? See example https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/defaul.... This is more of a peculiarity of Polymer v1 syntax, and less of Javascript. In essence everything defined in 'properties' is a Polymer property, allowing the HTML and JS code to interoperate. There is no need to use it for variables that are only used in JS (and not involved in any Polymer data bindings). https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:485: * @private On 2017/06/20 at 19:59:54, afakhry wrote: > On 2017/06/20 00:31:44, stevenjb wrote: > > @override > > It doesn't actually override, but rather shadows that function we get from the behavior. If I add @override, the closure compiler complains with: > > ## /work2/chromium/src/chrome/browser/resources/settings/device_page/night_light_slider.js:489: ERROR - property _createRipple not defined on any superclass of NightLightSliderElement > ## _createRipple: function() { > ## ^^^^^^^^^^^^ I don't think @override works for inherited behaviors.
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:76: this.$.startKnob.addEventListener('focus', this.onFocus_.bind(this)); On 2017/06/20 20:39:09, dpapad wrote: > On 2017/06/20 at 19:59:54, afakhry wrote: > > On 2017/06/20 01:24:47, dpapad wrote: > > > Why are those listeners added programmatically instead of declaratively? > > > > I have no idea. I tried that at first, but Chrome kept complaining with the > following error: > > > > "Refused to execute inline event handler because it violates the following > Content Security Policy directive: "script-src chrome://resources 'self' > 'unsafe-eval'". Either the 'unsafe-inline' keyword, a hash ('sha256-...'), or a > nonce ('nonce-...') is required to enable inline execution." > > That means that the declarative syntax was most likely incorrect. > > It should look roughly like this > <button on-tap="onTap_">....</button> > > where onTap_ is a method you define in the JS. See examples > https://cs.chromium.org/search/?q=%22on-tap%22+file:%5Esrc/chrome/browser/res.... Oh, I was using onfocus="onFocus_" instead of on-focus="onFocus_". Done. https://codereview.chromium.org/2950673002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:126: this.dragObject_ = this.$.startKnob; On 2017/06/20 20:39:09, dpapad wrote: > On 2017/06/20 at 19:59:54, afakhry wrote: > > On 2017/06/20 01:24:47, dpapad wrote: > > > Is dragObject_ used in the HTML? If not, there is no good reason to have it > as a > > > Polymer property. > > > > Sorry, I'm not very familiar with javascript. What's the alternative? Should I > define it somewhere in the file? Or should I just use it as is and it will be > added to the prototype of object 'this'? > > See example > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/defaul.... > This is more of a peculiarity of Polymer v1 syntax, and less of Javascript. > > In essence everything defined in 'properties' is a Polymer property, allowing > the HTML and JS code to interoperate. There is no need to use it for variables > that are only used in JS (and not involved in any Polymer data bindings). Thanks for the pointer and the explanation. Done.
LGTM with nits. Also suggesting prefixing the CL with "MD Settings" to make it more obvious what this CL is affecting. "Night light" is sort of a codename and not sufficient (at least I had no idea what this CL affects just by reading the description). https://codereview.chromium.org/2950673002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/device_page/night_light_slider.js:465: * @return {boolean} true if either of the two knobs is focused. Can we make this comment less verbose? /** * @return {boolean} Whether either of the two knobs is focused. * @private */
Description was changed from ========== [Night Light] CL8: Add the ripple focus effect to the slider knobs Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c20 BUG=705816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD Settings][Night Light] CL8: Add the ripple focus effect to the slider knobs Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c20 BUG=705816 ==========
https://codereview.chromium.org/2950673002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950673002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/device_page/night_light_slider.js:465: * @return {boolean} true if either of the two knobs is focused. On 2017/06/20 21:11:56, dpapad wrote: > Can we make this comment less verbose? > > /** > * @return {boolean} Whether either of the two knobs is focused. > * @private > */ Done.
Steven, could you please let me know if you have any final comments? Thanks!
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dpapad@ - thanks for the extra set of eyes / perspective. LGTM
The CQ bit was unchecked by afakhry@chromium.org
The CQ bit was checked by afakhry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2950673002/#ps100001 (title: "Nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498000107738510,
"parent_rev": "5ea087fcae356e2f872d63d4aed4aecf20169135", "commit_rev":
"151428415415bf5f90834e350fc53004668a9c8e"}
Message was sent while issue was closed.
Description was changed from ========== [MD Settings][Night Light] CL8: Add the ripple focus effect to the slider knobs Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c20 BUG=705816 ========== to ========== [MD Settings][Night Light] CL8: Add the ripple focus effect to the slider knobs Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c20 BUG=705816 Review-Url: https://codereview.chromium.org/2950673002 Cr-Commit-Position: refs/heads/master@{#481035} Committed: https://chromium.googlesource.com/chromium/src/+/151428415415bf5f90834e350fc5... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/151428415415bf5f90834e350fc5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
