Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2950393002:
[MD-Settings][Night Light] CL10: Add RTL languages support to the slider (Closed)
|
Created:
3 years, 6 months ago by afakhry Modified:
3 years, 6 months ago Reviewers:
stevenjb CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MD-Settings][Night Light] CL10: Add RTL languages support to the slider
Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c24
BUG=705816
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2950393002
Cr-Commit-Position: refs/heads/master@{#481783}
Committed: https://chromium.googlesource.com/chromium/src/+/b4bc987ccfa1400e505bcaae38fd7463af0b11ac
Patch Set 1 #
Total comments: 6
Patch Set 2 : Steven's comments #
Total comments: 4
Patch Set 3 : Making it a property #
Total comments: 2
Patch Set 4 : @private #
Messages
Total messages: 17 (6 generated)
Description was changed from ========== [MD-Settings][Night Light] CL10: Add RTL languages support to the slider Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c24 BUG=705816 ========== to ========== [MD-Settings][Night Light] CL10: Add RTL languages support to the slider Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c24 BUG=705816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
afakhry@chromium.org changed reviewers: + stevenjb@chromium.org
Steven, please review this CL. Thank you!
https://codereview.chromium.org/2950393002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950393002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:113: return this.isRTL_() ? style.rtl : style.ltr; Any particular reason not to pass 0-100 to this and return 'left: ' + (rtl ? 100 - percent : percent) + '%' instead of using a table? https://codereview.chromium.org/2950393002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:212: var ddx = this.isRTL_() ? (-1 * event.detail.ddx) : event.detail.ddx; nit: this.isRTL_() ? event.detail.ddx * -1 : event.detail.ddx; https://codereview.chromium.org/2950393002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:343: } nit: I think this would be more clear as: var rtl = this.isRTL_(); var endKnob = rtl ? this.$.startKnob : this.$.endKnob; etc
https://codereview.chromium.org/2950393002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950393002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:113: return this.isRTL_() ? style.rtl : style.ltr; On 2017/06/22 20:23:42, stevenjb wrote: > Any particular reason not to pass 0-100 to this and return 'left: ' + (rtl ? 100 > - percent : percent) + '%' instead of using a table? > No reason, other than I didn't want to do the calculation + string concatenation every time this function is called. Done. https://codereview.chromium.org/2950393002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:212: var ddx = this.isRTL_() ? (-1 * event.detail.ddx) : event.detail.ddx; On 2017/06/22 20:23:43, stevenjb wrote: > nit: this.isRTL_() ? event.detail.ddx * -1 : event.detail.ddx; > Done. https://codereview.chromium.org/2950393002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/device_page/night_light_slider.js:343: } On 2017/06/22 20:23:42, stevenjb wrote: > nit: I think this would be more clear as: > > var rtl = this.isRTL_(); > var endKnob = rtl ? this.$.startKnob : this.$.endKnob; > etc Done.
lgtm w/ one more suggestion https://codereview.chromium.org/2950393002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950393002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/night_light_slider.js:83: 'rtl'; This is an element so the following should work: window.getComputedStyle(this).direction == 'rtl' Also, we can/should probably just set this once in attached: /** @override */ attached: function() { this.isRTL_ = window.getComputedStyle(this).direction == 'rtl'; } (Declare isRL_ as a bool below dragObject_)
https://codereview.chromium.org/2950393002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950393002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/night_light_slider.js:83: 'rtl'; On 2017/06/22 21:17:22, stevenjb wrote: > This is an element so the following should work: > > window.getComputedStyle(this).direction == 'rtl' > > Also, we can/should probably just set this once in attached: > > /** @override */ > attached: function() { > this.isRTL_ = window.getComputedStyle(this).direction == 'rtl'; > } > > (Declare isRL_ as a bool below dragObject_) I tried all of this the first thing but it doesn't work for the legends unfortunately. getLegendStyle_() is called before attached(). Also calling window.getComputedStyle(this).direction in getLegendStyle_() doesn't work either! Apparently 'this' is not fully initialized yet? It only worked when I used: window.getComputedStyle(document.firstElementChild).direction
https://codereview.chromium.org/2950393002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950393002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/night_light_slider.js:83: 'rtl'; On 2017/06/22 21:51:26, afakhry wrote: > On 2017/06/22 21:17:22, stevenjb wrote: > > This is an element so the following should work: > > > > window.getComputedStyle(this).direction == 'rtl' > > > > Also, we can/should probably just set this once in attached: > > > > /** @override */ > > attached: function() { > > this.isRTL_ = window.getComputedStyle(this).direction == 'rtl'; > > } > > > > (Declare isRL_ as a bool below dragObject_) > > I tried all of this the first thing but it doesn't work for the legends > unfortunately. > > getLegendStyle_() is called before attached(). Also calling > > window.getComputedStyle(this).direction > > in getLegendStyle_() doesn't work either! Apparently 'this' is not fully > initialized yet? It only worked when I used: > > window.getComputedStyle(document.firstElementChild).direction You need to pass isRTL_ to the functions in the HTML to add it as a dependency. e.g. [[getLegendStyle_(0, isRTL_)]] That will defer the calling of getLegendStyle_ until isRTL_ is defined. (You'll also need to make isRTL_ a property). That should also fix the issue with passing |this| to getComputedStyle.
https://codereview.chromium.org/2950393002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950393002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/device_page/night_light_slider.js:83: 'rtl'; On 2017/06/22 22:12:05, stevenjb wrote: > On 2017/06/22 21:51:26, afakhry wrote: > > On 2017/06/22 21:17:22, stevenjb wrote: > > > This is an element so the following should work: > > > > > > window.getComputedStyle(this).direction == 'rtl' > > > > > > Also, we can/should probably just set this once in attached: > > > > > > /** @override */ > > > attached: function() { > > > this.isRTL_ = window.getComputedStyle(this).direction == 'rtl'; > > > } > > > > > > (Declare isRL_ as a bool below dragObject_) > > > > I tried all of this the first thing but it doesn't work for the legends > > unfortunately. > > > > getLegendStyle_() is called before attached(). Also calling > > > > window.getComputedStyle(this).direction > > > > in getLegendStyle_() doesn't work either! Apparently 'this' is not fully > > initialized yet? It only worked when I used: > > > > window.getComputedStyle(document.firstElementChild).direction > > You need to pass isRTL_ to the functions in the HTML to add it as a dependency. > e.g. [[getLegendStyle_(0, isRTL_)]] That will defer the calling of > getLegendStyle_ until isRTL_ is defined. (You'll also need to make isRTL_ a > property). That should also fix the issue with passing |this| to > getComputedStyle. > This works. Thank you very much for suggesting! It's confusing though! Done. Please take one final look.
lgtm https://codereview.chromium.org/2950393002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950393002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/night_light_slider.js:42: * Whether the window is in RTL locales. @private
https://codereview.chromium.org/2950393002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/device_page/night_light_slider.js (right): https://codereview.chromium.org/2950393002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/device_page/night_light_slider.js:42: * Whether the window is in RTL locales. On 2017/06/22 22:48:41, stevenjb wrote: > @private Done.
The CQ bit was checked by afakhry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2950393002/#ps60001 (title: "@private")
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": 60001, "attempt_start_ts": 1498172931392340,
"parent_rev": "5930ec69a322a83322e9a6976a5c9fc791429c4a", "commit_rev":
"b4bc987ccfa1400e505bcaae38fd7463af0b11ac"}
Message was sent while issue was closed.
Description was changed from ========== [MD-Settings][Night Light] CL10: Add RTL languages support to the slider Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c24 BUG=705816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD-Settings][Night Light] CL10: Add RTL languages support to the slider Demo: https://bugs.chromium.org/p/chromium/issues/detail?id=705816#c24 BUG=705816 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2950393002 Cr-Commit-Position: refs/heads/master@{#481783} Committed: https://chromium.googlesource.com/chromium/src/+/b4bc987ccfa1400e505bcaae38fd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/b4bc987ccfa1400e505bcaae38fd... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
