Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2638983002:
MD Settings: on people page, fix long names breaking arrow icon (Closed)
|
Created:
3 years, 11 months ago by scottchen Modified:
3 years, 11 months ago 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: on people page, fix long names breaking arrow icon
Long text inside a flex-child doesn't work properly with (text-)overflow due to min-width of a flex-child defaulting to the length of the content. A reasonably proper fix was found to be setting "min-width:0" on the flex-child to override this behavior, so that texts inside the flex-child are forced to ellipse.
BUG=680893, 680892, 680918
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2638983002
Cr-Commit-Position: refs/heads/master@{#445257}
Committed: https://chromium.googlesource.com/chromium/src/+/b8ff90663001db32c1cdbfb0df0c79dadb16d353
Patch Set 1 #
Total comments: 2
Messages
Total messages: 22 (13 generated)
Description was changed from ========== MD Settings: on people page, fix long names breaking arrow icon BUG=680893, 680892 ========== to ========== MD Settings: on people page, fix long names breaking arrow icon BUG=680893, 680892 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by scottchen@chromium.org to run a CQ dry run
On 2017/01/17 19:08:06, scottchen wrote: > Description was changed from > > ========== > MD Settings: on people page, fix long names breaking arrow icon > > BUG=680893, 680892 > ========== > > to > > ========== > MD Settings: on people page, fix long names breaking arrow icon > > BUG=680893, 680892 > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation > ========== http://imgur.com/a/9YIGS
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== MD Settings: on people page, fix long names breaking arrow icon BUG=680893, 680892 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: on people page, fix long names breaking arrow icon Long text inside a flex-child doesn't work properly with (text-)overflow due to min-width of a flex-child defaulting to the length of the content. A reasonably proper fix was found to be setting "min-width:0" on the flex-child to override this behavior, so that texts inside the flex-child are forced to ellipse. BUG=680893, 680892 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
scottchen@chromium.org changed reviewers: + dbeam@chromium.org, hcarmona@chromium.org
Description was changed from ========== MD Settings: on people page, fix long names breaking arrow icon Long text inside a flex-child doesn't work properly with (text-)overflow due to min-width of a flex-child defaulting to the length of the content. A reasonably proper fix was found to be setting "min-width:0" on the flex-child to override this behavior, so that texts inside the flex-child are forced to ellipse. BUG=680893, 680892 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: on people page, fix long names breaking arrow icon Long text inside a flex-child doesn't work properly with (text-)overflow due to min-width of a flex-child defaulting to the length of the content. A reasonably proper fix was found to be setting "min-width:0" on the flex-child to override this behavior, so that texts inside the flex-child are forced to ellipse. BUG=680893, 680892, 680918 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
https://codereview.chromium.org/2638983002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/2638983002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/people_page.html:111: min-width: 0; Can we use "overflow: hidden" here instead? It makes the code more readable and you wouldn't need to comment.
On 2017/01/19 01:53:31, hcarmona wrote: > https://codereview.chromium.org/2638983002/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/people_page/people_page.html (right): > > https://codereview.chromium.org/2638983002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/people_page/people_page.html:111: min-width: > 0; > Can we use "overflow: hidden" here instead? > It makes the code more readable and you wouldn't need to comment. using overflow: hidden will cut-off the paper-icon-button-light, since it actually slightly out of the container's bounds. Alternatively, we could adjust our styling so that .paper-icon-button-light doesn't need negative margin-end, but that would have a large impact surface (since its used a lot in settings) and might accidentally break something else easily.
https://codereview.chromium.org/2638983002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/2638983002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/people_page.html:111: min-width: 0; On 2017/01/19 01:53:31, hcarmona wrote: > Can we use "overflow: hidden" here instead? > It makes the code more readable and you wouldn't need to comment. using overflow: hidden will cut-off the paper-icon-button-light, since it actually slightly out of the container's bounds. Alternatively, we could adjust our styling so that .paper-icon-button-light doesn't need negative margin-end, but that would have a large impact surface (since its used a lot in settings) and might accidentally break something else easily.
On 2017/01/19 21:32:59, scottchen wrote: > https://codereview.chromium.org/2638983002/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/people_page/people_page.html (right): > > https://codereview.chromium.org/2638983002/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/people_page/people_page.html:111: min-width: > 0; > On 2017/01/19 01:53:31, hcarmona wrote: > > Can we use "overflow: hidden" here instead? > > It makes the code more readable and you wouldn't need to comment. > > using overflow: hidden will cut-off the paper-icon-button-light, since it > actually slightly out of the container's bounds. Alternatively, we could adjust > our styling so that .paper-icon-button-light doesn't need negative margin-end, > but that would have a large impact surface (since its used a lot in settings) > and might accidentally break something else easily. Oh, yes. I remember we talked about this. I think it we might want to take the hit and make sure this is done right instead of adding complexity that we'll have to pay later. We can double check w/ dbeam@ if this looks like too big a change. We can chat about this in person w/ him. It might be easier than going back/forth on the CL.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
lgtm
The CQ bit was checked by scottchen@chromium.org
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": 1, "attempt_start_ts": 1484963292459810, "parent_rev":
"6e10ee5b759fbe79d5e24096a337e9534012bbc8", "commit_rev":
"b8ff90663001db32c1cdbfb0df0c79dadb16d353"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: on people page, fix long names breaking arrow icon Long text inside a flex-child doesn't work properly with (text-)overflow due to min-width of a flex-child defaulting to the length of the content. A reasonably proper fix was found to be setting "min-width:0" on the flex-child to override this behavior, so that texts inside the flex-child are forced to ellipse. BUG=680893, 680892, 680918 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: on people page, fix long names breaking arrow icon Long text inside a flex-child doesn't work properly with (text-)overflow due to min-width of a flex-child defaulting to the length of the content. A reasonably proper fix was found to be setting "min-width:0" on the flex-child to override this behavior, so that texts inside the flex-child are forced to ellipse. BUG=680893, 680892, 680918 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2638983002 Cr-Commit-Position: refs/heads/master@{#445257} Committed: https://chromium.googlesource.com/chromium/src/+/b8ff90663001db32c1cdbfb0df0c... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/b8ff90663001db32c1cdbfb0df0c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
