Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2124783002:
Site Settings Desktop: Show menu right-aligned. (Closed)
|
Created:
4 years, 5 months ago by Finnur Modified:
4 years, 4 months ago Reviewers:
tommycli CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSite Settings Desktop: Show menu right-aligned.
BUG=625805
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/734a31f5d5cce5cf938445d7c1795dcddc8c69ca
Cr-Commit-Position: refs/heads/master@{#413728}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Sync #Messages
Total messages: 31 (16 generated)
Description was changed from ========== Site Settings Desktop: Show menu right-aligned. BUG=625805 ========== to ========== Site Settings Desktop: Show menu right-aligned. BUG=625805 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
finnur@chromium.org changed reviewers: + tommycli@chromium.org
https://codereview.chromium.org/2124783002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/2124783002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/site_list.html:54: <paper-menu-button hidden$="[[allSites]]" horizontal-align="right"> what about in RTL?
I modeled this after other code in MD-Settings, specifically one of the menus from the screenshot. Looking this value up on the Intertubes shows that possible values are 'left', 'right' and 'auto', with no mention of RTL. I was kind of hoping it worked similar to: https://github.com/PolymerElements/iron-fit-behavior/blob/master/iron-fit-beh... What is your recommendation?
On 2016/07/07 09:41:48, Finnur wrote: > I modeled this after other code in MD-Settings, specifically one of the menus > from the screenshot. Looking this value up on the Intertubes shows that possible > values are 'left', 'right' and 'auto', with no mention of RTL. I was kind of > hoping it worked similar to: > https://github.com/PolymerElements/iron-fit-behavior/blob/master/iron-fit-beh... i think it may use that code > > What is your recommendation? to try your patch in RTL ;)
https://codereview.chromium.org/2124783002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/2124783002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/site_list.html:54: <paper-menu-button hidden$="[[allSites]]" horizontal-align="right"> OK, circling back to this CL that I was working on before my vacation. I tested this on Windows with different locales and found the following: Currently (without my fix) the menu is anchored wrongly both for LTR and RTL (grows off page). Tried English, Arabic and Hebrew locales. With my fix LTR gets fixed but RTL seems to ignore this, as far as I can tell. To get RTL to alter the menu direction I tried various combinations for horizontal-align="x" including x=left, x=auto, x=<blank>. None seemed to have any effect. I'm beginning to wonder if this is a Polymer thing/bug. So what should we do? I'm tempted to check this in as-is (given we do exactly this elsewhere, it fixes LTR and leaves RTL no worse off), unless you guys know of options that work for both LTR and RTL?
What say you, guys?
lgtm see below https://codereview.chromium.org/2124783002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/site_settings/site_list.html (right): https://codereview.chromium.org/2124783002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/site_settings/site_list.html:54: <paper-menu-button hidden$="[[allSites]]" horizontal-align="right"> On 2016/08/19 16:13:30, Finnur wrote: > OK, circling back to this CL that I was working on before my vacation. > > I tested this on Windows with different locales and found the following: > > Currently (without my fix) the menu is anchored wrongly both for LTR and RTL > (grows off page). Tried English, Arabic and Hebrew locales. > > With my fix LTR gets fixed but RTL seems to ignore this, as far as I can tell. > To get RTL to alter the menu direction I tried various combinations for > horizontal-align="x" including x=left, x=auto, x=<blank>. None seemed to have > any effect. > > I'm beginning to wonder if this is a Polymer thing/bug. > > So what should we do? I'm tempted to check this in as-is (given we do exactly > this elsewhere, it fixes LTR and leaves RTL no worse off), unless you guys know > of options that work for both LTR and RTL? Fixing LTR while leaving RTL no worse off is fine with me so long as that bug remains open with a note that RTL is still wrong.
The CQ bit was checked by finnur@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Description was changed from ========== Site Settings Desktop: Show menu right-aligned. BUG=625805 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Site Settings Desktop: Show menu right-aligned. BUG=625805 ==========
The CQ bit was checked by finnur@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== Site Settings Desktop: Show menu right-aligned. BUG=625805 ========== to ========== Site Settings Desktop: Show menu right-aligned. BUG=625805 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2124783002/#ps40001 (title: "Sync")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by finnur@chromium.org
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2124783002/#ps60001 (title: "Sync")
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.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Site Settings Desktop: Show menu right-aligned. BUG=625805 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Site Settings Desktop: Show menu right-aligned. BUG=625805 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/734a31f5d5cce5cf938445d7c1795dcddc8c69ca Cr-Commit-Position: refs/heads/master@{#413728} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/734a31f5d5cce5cf938445d7c1795dcddc8c69ca Cr-Commit-Position: refs/heads/master@{#413728} |
