Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2955053003:
Line-wrap Reader Mode infobar if the text is too long (Closed)
DescriptionLine-wrap Reader Mode infobar if the text is too long
The message text in the Reader Mode infobar cannot fit into the
current space if the screen is small, font is large, or the string
in the locale is long. From now on, it would wrap into multiple
lines in that case.
The infobar would become taller if more than one line is needed.
BUG=560118
Review-Url: https://codereview.chromium.org/2955053003
Cr-Commit-Position: refs/heads/master@{#485042}
Committed: https://chromium.googlesource.com/chromium/src/+/7accad67a9abf16cb043bc4affb26332e90454ff
Patch Set 1 #
Total comments: 2
Patch Set 2 : move padding to reader mode #
Total comments: 2
Patch Set 3 : rename infobar_text_padding #Patch Set 4 : rebase #
Messages
Total messages: 36 (17 generated)
wychen@chromium.org changed reviewers: + mdjones@chromium.org, twellington@chromium.org
PTAL
https://codereview.chromium.org/2955053003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarCompactLayout.java (right): https://codereview.chromium.org/2955053003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarCompactLayout.java:65: view.setPadding(0, mMessagePadding, 0, mMessagePadding); Why is this now needed? Does it affect the other compact infobars?
https://codereview.chromium.org/2955053003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarCompactLayout.java (right): https://codereview.chromium.org/2955053003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBarCompactLayout.java:65: view.setPadding(0, mMessagePadding, 0, mMessagePadding); On 2017/06/27 20:17:18, mdjones wrote: > Why is this now needed? Does it affect the other compact infobars? If the text fits in one line, the infobar is tall enough so that it naturally has large white space between message and the border of infobars. When it is wrapped, the space is too small, so padding is needed to make it look good. This doesn't affect the looks if the text fits in one line, but it does affect compact translation infobar. Moved to Reader Mode only.
Description was changed from ========== Line-wrap Reader Mode infobar if the text is too long The infobar would become taller if more than one line is needed. BUG=560118 ========== to ========== Line-wrap Reader Mode infobar if the text is too long The message text in the Reader Mode infobar cannot fit into the current space if the screen is small, font is large, or the string in the locale is long. From now on, it would wrap into multiple lines in that case. The infobar would become taller if more than one line is needed. BUG=560118 ==========
lgtm % nit https://codereview.chromium.org/2955053003/diff/20001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2955053003/diff/20001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:67: <dimen name="infobar_text_padding">8dp</dimen> nit: make this reader mode specific too.
https://codereview.chromium.org/2955053003/diff/20001/chrome/android/java/res... File chrome/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2955053003/diff/20001/chrome/android/java/res... chrome/android/java/res/values/dimens.xml:67: <dimen name="infobar_text_padding">8dp</dimen> On 2017/06/27 21:54:28, mdjones wrote: > nit: make this reader mode specific too. Good catch!
Theresa, can you take a look? Thanks!
rs lgtm
The CQ bit was checked by wychen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mdjones@chromium.org Link to the patchset: https://codereview.chromium.org/2955053003/#ps40001 (title: "rename infobar_text_padding")
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by wychen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mdjones@chromium.org, twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2955053003/#ps60001 (title: "rebase")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by wychen@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by wychen@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by wychen@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by wychen@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": 60001, "attempt_start_ts": 1499457634696020,
"parent_rev": "807879a191895d9acc2a9230c3fadfc5273cfd38", "commit_rev":
"7accad67a9abf16cb043bc4affb26332e90454ff"}
Message was sent while issue was closed.
Description was changed from ========== Line-wrap Reader Mode infobar if the text is too long The message text in the Reader Mode infobar cannot fit into the current space if the screen is small, font is large, or the string in the locale is long. From now on, it would wrap into multiple lines in that case. The infobar would become taller if more than one line is needed. BUG=560118 ========== to ========== Line-wrap Reader Mode infobar if the text is too long The message text in the Reader Mode infobar cannot fit into the current space if the screen is small, font is large, or the string in the locale is long. From now on, it would wrap into multiple lines in that case. The infobar would become taller if more than one line is needed. BUG=560118 Review-Url: https://codereview.chromium.org/2955053003 Cr-Commit-Position: refs/heads/master@{#485042} Committed: https://chromium.googlesource.com/chromium/src/+/7accad67a9abf16cb043bc4affb2... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7accad67a9abf16cb043bc4affb2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
