Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1834033002:
Make the passwords list have 3 columns and not over-scroll. (Closed)
|
Created:
4 years, 9 months ago by hcarmona Modified:
4 years, 8 months ago Reviewers:
michaelpg 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@password-edit-dialog.gitbr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake the passwords list have 3 columns and not over-scroll.
Also moves CSS to HTML to match other, newer, elements.
Screenshot is attached to bug.
BUG=595574
Committed: https://crrev.com/7e2be002b0a51a571d13e0724020f684c8ea80b8
Cr-Commit-Position: refs/heads/master@{#384361}
Patch Set 1 #Patch Set 2 : UX Feedback Applied #
Total comments: 7
Patch Set 3 : Resolve TODO #
Total comments: 2
Patch Set 4 : Remove Flex #
Messages
Total messages: 22 (8 generated)
Description was changed from ========== Make the passwords list have 3 columns and not over-scroll. Screenshot is attached to bug. BUG=595574 ========== to ========== Make the passwords list have 3 columns and not over-scroll. Also moves CSS to HTML to match other, newer, elements. Screenshot is attached to bug. BUG=595574 ==========
hcarmona@chromium.org changed reviewers: + michaelpg@chromium.org
PTAL Dependency on other CL is for i18n values. Thanks!
Got feedback from bettes@ on the screenshot, I will be updating the CSS. No need to review until it's updated. Thanks!
Patchset #2 (id:20001) has been deleted
Updated based on conversation w/ bettes@ Updated screenshot is in the bug.
https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:32: width: 0; doesn't this hide the input?
https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:32: width: 0; On 2016/03/29 03:54:22, michaelpg wrote: > doesn't this hide the input? width 0 makes it start small, but flex makes it grow. The password field is too wide without this.
https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:32: width: 0; On 2016/03/29 19:20:18, Hector Carmona wrote: > On 2016/03/29 03:54:22, michaelpg wrote: > > doesn't this hide the input? > > width 0 makes it start small, but flex makes it grow. > The password field is too wide without this. that's confusing. is this something we do elsewhere? can we use something like flex-basis instead?
https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:32: width: 0; On 2016/03/29 22:00:01, michaelpg wrote: > On 2016/03/29 19:20:18, Hector Carmona wrote: > > On 2016/03/29 03:54:22, michaelpg wrote: > > > doesn't this hide the input? > > > > width 0 makes it start small, but flex makes it grow. > > The password field is too wide without this. > > that's confusing. is this something we do elsewhere? can we use something like > flex-basis instead? Ok, I've spent some time looking into this. I can't find anywhere in the code where we do this. I tried flex-basis and spent some time researching this and created this jsbin to test on: http://jsbin.com/laqimuheve/7/edit?html,css,output Width 0 and flex 1 are the only thing I could get to work for an input element. The reason I need the input is to show password formatted text, so an alternative would be to use bullet characters in a regular div. What do you think? Reason min-width didn't work is because input fields have a default content width. :-/ https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:116: <!-- TODO(hcarmona): fix menu placement... it's too left --> Forgot to remove this TODO. Menu works.
https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:32: width: 0; On 2016/03/30 21:32:18, Hector Carmona wrote: > On 2016/03/29 22:00:01, michaelpg wrote: > > On 2016/03/29 19:20:18, Hector Carmona wrote: > > > On 2016/03/29 03:54:22, michaelpg wrote: > > > > doesn't this hide the input? > > > > > > width 0 makes it start small, but flex makes it grow. > > > The password field is too wide without this. > > > > that's confusing. is this something we do elsewhere? can we use something like > > flex-basis instead? > > Ok, I've spent some time looking into this. I can't find anywhere in the > code where we do this. I tried flex-basis and spent some time > researching this and created this jsbin to test on: > http://jsbin.com/laqimuheve/7/edit?html,css,output > > Width 0 and flex 1 are the only thing I could get to work for an input > element. The reason I need the input is to show password formatted text, > so an alternative would be to use bullet characters in a regular div. > > What do you think? Reason min-width didn't work is because input fields > have a default content width. :-/ I haven't been tracking this convo closely, but `width: auto; min-width: 0;` might remove a default width and achieve what you want?
On 2016/03/30 21:59:21, Dan Beam wrote: > https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... > File > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html > (right): > > https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:32: > width: 0; > On 2016/03/30 21:32:18, Hector Carmona wrote: > > On 2016/03/29 22:00:01, michaelpg wrote: > > > On 2016/03/29 19:20:18, Hector Carmona wrote: > > > > On 2016/03/29 03:54:22, michaelpg wrote: > > > > > doesn't this hide the input? > > > > > > > > width 0 makes it start small, but flex makes it grow. > > > > The password field is too wide without this. > > > > > > that's confusing. is this something we do elsewhere? can we use something > like > > > flex-basis instead? > > > > Ok, I've spent some time looking into this. I can't find anywhere in the > > code where we do this. I tried flex-basis and spent some time > > researching this and created this jsbin to test on: > > http://jsbin.com/laqimuheve/7/edit?html,css,output > > > > Width 0 and flex 1 are the only thing I could get to work for an input > > element. The reason I need the input is to show password formatted text, > > so an alternative would be to use bullet characters in a regular div. > > > > What do you think? Reason min-width didn't work is because input fields > > have a default content width. :-/ > > I haven't been tracking this convo closely, but `width: auto; min-width: 0;` > might remove a default width and achieve what you want? No luck, just tried that on the jsbin
lgtm https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1834033002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:32: width: 0; On 2016/03/30 21:59:21, Dan Beam wrote: > On 2016/03/30 21:32:18, Hector Carmona wrote: > > On 2016/03/29 22:00:01, michaelpg wrote: > > > On 2016/03/29 19:20:18, Hector Carmona wrote: > > > > On 2016/03/29 03:54:22, michaelpg wrote: > > > > > doesn't this hide the input? > > > > > > > > width 0 makes it start small, but flex makes it grow. > > > > The password field is too wide without this. > > > > > > that's confusing. is this something we do elsewhere? can we use something > like > > > flex-basis instead? > > > > Ok, I've spent some time looking into this. I can't find anywhere in the > > code where we do this. I tried flex-basis and spent some time > > researching this and created this jsbin to test on: > > http://jsbin.com/laqimuheve/7/edit?html,css,output > > > > Width 0 and flex 1 are the only thing I could get to work for an input > > element. The reason I need the input is to show password formatted text, > > so an alternative would be to use bullet characters in a regular div. > > > > What do you think? Reason min-width didn't work is because input fields > > have a default content width. :-/ > > I haven't been tracking this convo closely, but `width: auto; min-width: 0;` > might remove a default width and achieve what you want? Seems like <input> is just weird when it comes to width. width: 0 will be fine I guess. https://codereview.chromium.org/1834033002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1834033002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:87: <div class="saved-password-columns list-item" flex> this and below: set flex in CSS, or import iron-flex-layout/iron-flex-layout.html and iron-flex-layout/classes/iron-flex-layout.html
Patchset #4 (id:80001) has been deleted
Removed flex and updated font-weight on heading and color on icons based on bettes@ feedback on last screenshot. Updated screenshot in bug. https://codereview.chromium.org/1834033002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html (right): https://codereview.chromium.org/1834033002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/passwords_section.html:87: <div class="saved-password-columns list-item" flex> On 2016/03/30 22:44:41, michaelpg wrote: > this and below: set flex in CSS, or import > iron-flex-layout/iron-flex-layout.html and > iron-flex-layout/classes/iron-flex-layout.html list-item has display: flex. removing flex attribute.
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1834033002/#ps100001 (title: "Remove Flex")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1834033002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1834033002/100001
Message was sent while issue was closed.
Description was changed from ========== Make the passwords list have 3 columns and not over-scroll. Also moves CSS to HTML to match other, newer, elements. Screenshot is attached to bug. BUG=595574 ========== to ========== Make the passwords list have 3 columns and not over-scroll. Also moves CSS to HTML to match other, newer, elements. Screenshot is attached to bug. BUG=595574 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Make the passwords list have 3 columns and not over-scroll. Also moves CSS to HTML to match other, newer, elements. Screenshot is attached to bug. BUG=595574 ========== to ========== Make the passwords list have 3 columns and not over-scroll. Also moves CSS to HTML to match other, newer, elements. Screenshot is attached to bug. BUG=595574 Committed: https://crrev.com/7e2be002b0a51a571d13e0724020f684c8ea80b8 Cr-Commit-Position: refs/heads/master@{#384361} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7e2be002b0a51a571d13e0724020f684c8ea80b8 Cr-Commit-Position: refs/heads/master@{#384361} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
