Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2213833002:
Fix files-tooltip to consider RTL. (Closed)
|
Created:
4 years, 4 months ago by harukam Modified:
4 years, 4 months ago Reviewers:
fukino CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix files-tooltip to consider RTL.
BUG=617517
TEST=manually tested
Committed: https://crrev.com/c40c0d3c737df5e2a8cfd3582e431a38295f33b7
Cr-Commit-Position: refs/heads/master@{#409743}
Patch Set 1 #
Total comments: 4
Patch Set 2 : addressed a comment #Messages
Total messages: 14 (5 generated)
Description was changed from ========== Fix files-tooltip to consider RTL. BUG=617517 ========== to ========== Fix files-tooltip to consider RTL. BUG=617517 TEST=manually tested ==========
harukam@google.com changed reviewers: + fukino@chromium.org
PTAL
https://codereview.chromium.org/2213833002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/foreground/elements/files_tooltip.js (right): https://codereview.chromium.org/2213833002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/foreground/elements/files_tooltip.js:173: left = 0; I think the |left| value should be assigned to this.style.right in RTL language. To know the current direction, you can use isRTL() defined in https://codesearch.chromium.org/chromium/src/ui/webui/resources/js/util.js?q=...
https://codereview.chromium.org/2213833002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/foreground/elements/files_tooltip.js (right): https://codereview.chromium.org/2213833002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/foreground/elements/files_tooltip.js:173: left = 0; On 2016/08/04 07:43:41, fukino wrote: > I think the |left| value should be assigned to this.style.right in RTL language. > To know the current direction, you can use isRTL() defined in > https://codesearch.chromium.org/chromium/src/ui/webui/resources/js/util.js?q=... Sorry! Your code is right. Forget about the previous comment. Using Math.max() and Math.min() might be more natural to limit the value range. like: left = Math.max(0, Math.min(left, document.body.offsetWidth - this.offsetWidth));
https://codereview.chromium.org/2213833002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/foreground/elements/files_tooltip.js (right): https://codereview.chromium.org/2213833002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/foreground/elements/files_tooltip.js:173: left = 0; On 2016/08/04 07:49:57, fukino wrote: > On 2016/08/04 07:43:41, fukino wrote: > > I think the |left| value should be assigned to this.style.right in RTL > language. > > To know the current direction, you can use isRTL() defined in > > > https://codesearch.chromium.org/chromium/src/ui/webui/resources/js/util.js?q=... > > Sorry! Your code is right. Forget about the previous comment. > > Using Math.max() and Math.min() might be more natural to limit the value range. > like: > left = Math.max(0, Math.min(left, document.body.offsetWidth - > this.offsetWidth)); Or, if (x < MIN) x = MIN; if (x > MAX) x = MAX;
PTAL https://codereview.chromium.org/2213833002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/foreground/elements/files_tooltip.js (right): https://codereview.chromium.org/2213833002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/foreground/elements/files_tooltip.js:173: left = 0; On 2016/08/04 08:05:52, fukino wrote: > On 2016/08/04 07:49:57, fukino wrote: > > On 2016/08/04 07:43:41, fukino wrote: > > > I think the |left| value should be assigned to this.style.right in RTL > > language. > > > To know the current direction, you can use isRTL() defined in > > > > > > https://codesearch.chromium.org/chromium/src/ui/webui/resources/js/util.js?q=... > > > > Sorry! Your code is right. Forget about the previous comment. > > > > Using Math.max() and Math.min() might be more natural to limit the value > range. > > like: > > left = Math.max(0, Math.min(left, document.body.offsetWidth - > > this.offsetWidth)); > > Or, > if (x < MIN) > x = MIN; > if (x > MAX) > x = MAX; Done.
lgtm
The CQ bit was checked by harukam@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix files-tooltip to consider RTL. BUG=617517 TEST=manually tested ========== to ========== Fix files-tooltip to consider RTL. BUG=617517 TEST=manually tested ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix files-tooltip to consider RTL. BUG=617517 TEST=manually tested ========== to ========== Fix files-tooltip to consider RTL. BUG=617517 TEST=manually tested Committed: https://crrev.com/c40c0d3c737df5e2a8cfd3582e431a38295f33b7 Cr-Commit-Position: refs/heads/master@{#409743} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c40c0d3c737df5e2a8cfd3582e431a38295f33b7 Cr-Commit-Position: refs/heads/master@{#409743} |
