Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2219803003:
MD Settings: About page, force line breaks in "Command line" string. (Closed)
|
Created:
4 years, 4 months ago by dpapad Modified:
4 years, 4 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: About page, force line breaks in "Command line" string.
BUG=635020
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/070f89c9808629a65f02a0f3c818a31c873b3e9e
Cr-Commit-Position: refs/heads/master@{#410177}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Nit. #Patch Set 3 : Tweak to use speced only properties. #Messages
Total messages: 21 (8 generated)
Description was changed from ========== MD Settings: About page, force line breaks in "Command line" string. BUG=635020 ========== to ========== MD Settings: About page, force line breaks in "Command line" string. BUG=635020 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dpapad@chromium.org changed reviewers: + michaelpg@chromium.org
Screenshot at http://imgur.com/a/THxqY.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
lgtm https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/detailed_build_info.html (right): https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/detailed_build_info.html:19: don't have any spaces, need to force a line break in such cases. */ nit: /* The command line ... * don't have any spaces, ... .*/ ^
lgtm https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/detailed_build_info.html (right): https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/detailed_build_info.html:21: word-break: break-word; suggestion: use overflow-wrap (break-word is not a standardized value for word-break)
https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/detailed_build_info.html (right): https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/detailed_build_info.html:19: don't have any spaces, need to force a line break in such cases. */ On 2016/08/05 at 19:37:42, Dan Beam wrote: > nit: > > /* The command line ... > * don't have any spaces, ... .*/ > ^ Done. https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/detailed_build_info.html:21: word-break: break-word; On 2016/08/05 at 19:58:37, michaelpg wrote: > suggestion: use overflow-wrap (break-word is not a standardized value for word-break) Tried this but did not work, see http://imgur.com/a/QBTVC.
https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/detailed_build_info.html (right): https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/detailed_build_info.html:21: word-break: break-word; On 2016/08/05 at 20:17:41, dpapad wrote: > On 2016/08/05 at 19:58:37, michaelpg wrote: > > suggestion: use overflow-wrap (break-word is not a standardized value for word-break) > > Tried this but did not work, see http://imgur.com/a/QBTVC. Done. So after experimenting with overflow-wrap, it only works if the width is specified, so added a width: 100% and it all works now.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2219803003/#ps40001 (title: "Tweak to use speced only properties.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/detailed_build_info.html (right): https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/detailed_build_info.html:21: word-break: break-word; On 2016/08/05 20:35:00, dpapad wrote: > On 2016/08/05 at 20:17:41, dpapad wrote: > > On 2016/08/05 at 19:58:37, michaelpg wrote: > > > suggestion: use overflow-wrap (break-word is not a standardized value for > word-break) > > > > Tried this but did not work, see http://imgur.com/a/QBTVC. > > Done. So after experimenting with overflow-wrap, it only works if the width is > specified, so added a width: 100% and it all works now. Wouldn't width: 100% have to be on the parent, not this div, to stop it from stretching? overflow: hidden on the parent would also work.
The CQ bit was unchecked by michaelpg@chromium.org
unchecking CQ bit because this doesn't fix the issue on my actual chromebook
https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/detailed_build_info.html (right): https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/detailed_build_info.html:21: word-break: break-word; On 2016/08/05 at 20:46:17, michaelpg wrote: > On 2016/08/05 20:35:00, dpapad wrote: > > On 2016/08/05 at 20:17:41, dpapad wrote: > > > On 2016/08/05 at 19:58:37, michaelpg wrote: > > > > suggestion: use overflow-wrap (break-word is not a standardized value for > > word-break) > > > > > > Tried this but did not work, see http://imgur.com/a/QBTVC. > > > > Done. So after experimenting with overflow-wrap, it only works if the width is > > specified, so added a width: 100% and it all works now. > > Wouldn't width: 100% have to be on the parent, not this div, to stop it from stretching? width: 100% on the parent has a different effect, see http://imgur.com/a/UKzRA. Not sure what you mean with "stop it from stretching". 100% on the div itself means "take as much space as your ancestors allow you", which is what desired here. > > overflow: hidden on the parent would also work. Does not seem to work. Even if it did, why would this be better? I would need to add a 2nd ID to apply the overflow: hidden on the parent, whereas with current solution all CSS is applied on a single element.
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/detailed_build_info.html (right): https://codereview.chromium.org/2219803003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/detailed_build_info.html:21: word-break: break-word; On 2016/08/05 20:58:21, dpapad wrote: > On 2016/08/05 at 20:46:17, michaelpg wrote: > > On 2016/08/05 20:35:00, dpapad wrote: > > > On 2016/08/05 at 20:17:41, dpapad wrote: > > > > On 2016/08/05 at 19:58:37, michaelpg wrote: > > > > > suggestion: use overflow-wrap (break-word is not a standardized value > for > > > word-break) > > > > > > > > Tried this but did not work, see http://imgur.com/a/QBTVC. > > > > > > Done. So after experimenting with overflow-wrap, it only works if the width > is > > > specified, so added a width: 100% and it all works now. > > > > Wouldn't width: 100% have to be on the parent, not this div, to stop it from > stretching? > width: 100% on the parent has a different effect, see http://imgur.com/a/UKzRA. > Not sure what you mean with "stop it from stretching". 100% on the div itself > means "take as much space as your ancestors allow you", which is what desired > here. > > > > > overflow: hidden on the parent would also work. > Does not seem to work. Even if it did, why would this be better? I would need to > add a 2nd ID to apply the overflow: hidden on the parent, whereas with current > solution all CSS is applied on a single element. you're right. dev mode on Samus is apparently a month behind right now :-( so I'm looking at a different set of divs! https://codereview.chromium.org/2169193003
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: About page, force line breaks in "Command line" string. BUG=635020 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: About page, force line breaks in "Command line" string. BUG=635020 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/070f89c9808629a65f02a0f3c818a31c873b3e9e Cr-Commit-Position: refs/heads/master@{#410177} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/070f89c9808629a65f02a0f3c818a31c873b3e9e Cr-Commit-Position: refs/heads/master@{#410177} |
