Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2817243003:
[MD settings] observe changes to default font size in Appearance (Closed)
|
Created:
3 years, 8 months ago by dschuyler Modified:
3 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 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] observe changes to default font size in Appearance
This CL updates the fixed font size from the Appearance section reliably.
(It was previously working depending on which settings page had been
visited).
Also, updates the UI to remove the font size on the example of the fixed
font size.
BUG=671562
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2817243003
Cr-Commit-Position: refs/heads/master@{#465054}
Committed: https://chromium.googlesource.com/chromium/src/+/b7cd6a5130a7a261ae997c5e37dc4fec4560cff3
Patch Set 1 #
Total comments: 10
Patch Set 2 : merge with master #Patch Set 3 : nits #
Messages
Total messages: 24 (19 generated)
Description was changed from ========== [MD settings] observe changes to default font size in Appearance This CL updates the fixed font size from the Appearance section reliably. (It was previously working depending on which settings page had been visited). BUG=671562 ========== to ========== [MD settings] observe changes to default font size in Appearance This CL updates the fixed font size from the Appearance section reliably. (It was previously working depending on which settings page had been visited). BUG=671562 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
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] observe changes to default font size in Appearance This CL updates the fixed font size from the Appearance section reliably. (It was previously working depending on which settings page had been visited). BUG=671562 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] observe changes to default font size in Appearance This CL updates the fixed font size from the Appearance section reliably. (It was previously working depending on which settings page had been visited). Also, updates the UI to remove the font size on the example of the fixed font size. BUG=671562 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dschuyler@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html (right): https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:115: [[prefs.webkit.webprefs.default_fixed_font_size.value]]px; This is part of the 'also fixed' an not the primary fix in this CL. This wasn't actually showing the example in the correct font size. https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:118: $i18n{quickBrownFox} So it looks strange to show that this is 3px smaller than the others. I looked to see what was done previously and the font size is simply not shown for the fixed font size, which is less distracting. https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/appearance_page/appearance_fonts_page.js (left): https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_fonts_page.js:155: value - SIZE_DIFFERENCE_FIXED_STANDARD_); Chatted with michaelpg@ about this off-line. https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_page.js:125: // <if expr="is_linux and not chromeos"> This indention (here and below) is done by git cl format --js
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html (right): https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:118: $i18n{quickBrownFox} On 2017/04/15 01:27:57, dschuyler wrote: > So it looks strange to show that this is 3px smaller than the others. I looked > to see what was done previously and the font size is simply not shown for the > fixed font size, which is less distracting. SGTM. It would be more consistent to remove the prefixes on the other samples, too, but we'd need to show it somewhere (the font size row, maybe?) and that's a whole 'nother UX decision. https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_page.js:122: 'onDefaultFontSizeChanged_(prefs.webkit.webprefs.default_font_size.value)', nit: consistency w/ other observers: defaultFontSizeChanged_ https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_page.js:183: // This is unusual but there is a pref that is dependent upon another. as a *minor* nit, could you try to condense this comment a bit?
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html (right): https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_fonts_page.html:118: $i18n{quickBrownFox} On 2017/04/17 20:06:28, michaelpg wrote: > On 2017/04/15 01:27:57, dschuyler wrote: > > So it looks strange to show that this is 3px smaller than the others. I looked > > to see what was done previously and the font size is simply not shown for the > > fixed font size, which is less distracting. > > SGTM. It would be more consistent to remove the prefixes on the other samples, > too, but we'd need to show it somewhere (the font size row, maybe?) and that's a > whole 'nother UX decision. Acknowledged. https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_page.js:122: 'onDefaultFontSizeChanged_(prefs.webkit.webprefs.default_font_size.value)', On 2017/04/17 20:06:28, michaelpg wrote: > nit: consistency w/ other observers: defaultFontSizeChanged_ Done. https://codereview.chromium.org/2817243003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/appearance_page/appearance_page.js:183: // This is unusual but there is a pref that is dependent upon another. On 2017/04/17 20:06:28, michaelpg wrote: > as a *minor* nit, could you try to condense this comment a bit? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@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/2817243003/#ps40001 (title: "nits")
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": 40001, "attempt_start_ts": 1492470470379460,
"parent_rev": "fa3c7ef668ab513135b19f9d35ae9cd7cec84960", "commit_rev":
"b7cd6a5130a7a261ae997c5e37dc4fec4560cff3"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] observe changes to default font size in Appearance This CL updates the fixed font size from the Appearance section reliably. (It was previously working depending on which settings page had been visited). Also, updates the UI to remove the font size on the example of the fixed font size. BUG=671562 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] observe changes to default font size in Appearance This CL updates the fixed font size from the Appearance section reliably. (It was previously working depending on which settings page had been visited). Also, updates the UI to remove the font size on the example of the fixed font size. BUG=671562 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2817243003 Cr-Commit-Position: refs/heads/master@{#465054} Committed: https://chromium.googlesource.com/chromium/src/+/b7cd6a5130a7a261ae997c5e37dc... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b7cd6a5130a7a261ae997c5e37dc... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
