Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1380743003:
[MD settings] adding web content settings strings (Closed)
|
Created:
5 years, 2 months ago by dschuyler Modified:
5 years, 2 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_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. |
Description[MD settings] adding web content settings strings
This CL adds strings that will be used in the WebContent
settings UI.
BUG=538372
Committed: https://crrev.com/b43479fa6ac8e7f54871af30a78b66b0e3fca2a4
Cr-Commit-Position: refs/heads/master@{#353451}
Patch Set 1 #Patch Set 2 : merge with master #Patch Set 3 : fix typo #
Total comments: 12
Patch Set 4 : review changes #
Total comments: 2
Patch Set 5 : review nit #Patch Set 6 : merge with master #Patch Set 7 : changed case of page title #
Messages
Total messages: 19 (5 generated)
dschuyler@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:547: Very Small should this be title case (Very Small) or normal case (Very small)? does it make a difference on Mac vs. other platforms? https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:559: Very Large same question https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:567: <message name="IDS_SETTING_FONTS_AND_ENCODING" desc="Title for the customize fonts page in settings."> here and below.. IDS_SETTING -> IDS_SETTINGS https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:594: <message name="IDS_SETTING_LOREM_IPSUM" desc="Jibberish example text in settings."> Gibberish https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:597: <message name="IDS_SETTING_LOADING" desc="Placholder text while waiting for font settings."> so, many of your message names are very general ("IDS_SETTINGS_LOADING") but the descriptions are very specific. If you want general messages (which IDS_SETTINGS_LOADING implies), better to make the descriptions more general so we can more safely reuse these, e.g., desc="Placeholder text shown while waiting for a certain group of settings to load." If you want messages to be restricted to specific use cases in case it affects translations, use a more "namespaced" message name, e.g., IDS_SETTINGS_FONT_SIZE_TINY_LABEL.
chantellb30@gmail.com changed reviewers: + chantellb30@gmail.com
lgtm
https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:547: Very Small On 2015/10/03 05:15:04, michaelpg (OOO til 19th) wrote: > should this be title case (Very Small) or normal case (Very small)? does it make > a difference on Mac vs. other platforms? In the existing settings they are up-case for both words on both Mac and Linux. I asked Alan about it, he said they should be "Very small" and "Very large", i.e. sentence case. Done. https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:559: Very Large On 2015/10/03 05:15:03, michaelpg (OOO til 19th) wrote: > same question Acknowledged. https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:567: <message name="IDS_SETTING_FONTS_AND_ENCODING" desc="Title for the customize fonts page in settings."> On 2015/10/03 05:15:04, michaelpg (OOO til 19th) wrote: > here and below.. IDS_SETTING -> IDS_SETTINGS Done. https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:594: <message name="IDS_SETTING_LOREM_IPSUM" desc="Jibberish example text in settings."> On 2015/10/03 05:15:04, michaelpg (OOO til 19th) wrote: > Gibberish Done. https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:597: <message name="IDS_SETTING_LOADING" desc="Placholder text while waiting for font settings."> On 2015/10/03 05:15:04, michaelpg (OOO til 19th) wrote: > so, many of your message names are very general ("IDS_SETTINGS_LOADING") but the > descriptions are very specific. > > If you want general messages (which IDS_SETTINGS_LOADING implies), better to > make the descriptions more general so we can more safely reuse these, e.g., > desc="Placeholder text shown while waiting for a certain group of settings to > load." > > If you want messages to be restricted to specific use cases in case it affects > translations, use a more "namespaced" message name, e.g., > IDS_SETTINGS_FONT_SIZE_TINY_LABEL. Done.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:547: Very Small On 2015/10/05 18:18:57, dschuyler wrote: > On 2015/10/03 05:15:04, michaelpg (OOO til 19th) wrote: > > should this be title case (Very Small) or normal case (Very small)? does it > make > > a difference on Mac vs. other platforms? > > In the existing settings they are up-case for both words on both Mac and Linux. > I asked Alan about it, he said they should be "Very small" and "Very large", > i.e. > sentence case. > > Done. <if expr="use_titlecase"> Very Small </if> <if expr="not use_titlecase"> Very small </if> https://code.google.com/p/chromium/codesearch#chromium/src/docs/chrome_settin...
https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/1380743003/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:547: Very Small On 2015/10/05 18:31:20, Dan Beam wrote: > On 2015/10/05 18:18:57, dschuyler wrote: > > On 2015/10/03 05:15:04, michaelpg (OOO til 19th) wrote: > > > should this be title case (Very Small) or normal case (Very small)? does it > > make > > > a difference on Mac vs. other platforms? > > > > In the existing settings they are up-case for both words on both Mac and > Linux. > > I asked Alan about it, he said they should be "Very small" and "Very large", > > i.e. > > sentence case. > > > > Done. > > <if expr="use_titlecase"> > Very Small > </if> > <if expr="not use_titlecase"> > Very small > </if> > > https://code.google.com/p/chromium/codesearch#chromium/src/docs/chrome_settin... (also beware of this ^ page because it's ancient)
i don't see any titlecase in old IDS_OPTIONS*, so we should probably only do sentence case for now
lgtm https://codereview.chromium.org/1380743003/diff/60001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/1380743003/diff/60001/chrome/app/settings_str... chrome/app/settings_strings.grdp:597: <message name="IDS_SETTINGS_LOADING" desc="Placholder text while waiting for disk or network IO in settings."> I think this went overly technical. "Placeholder text while waiting for font settings to appear, shown only until font settings have loaded." (just a suggestion)
https://codereview.chromium.org/1380743003/diff/60001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/1380743003/diff/60001/chrome/app/settings_str... chrome/app/settings_strings.grdp:597: <message name="IDS_SETTINGS_LOADING" desc="Placholder text while waiting for disk or network IO in settings."> On 2015/10/07 17:59:56, michaelpg (OOO til 19th) wrote: > I think this went overly technical. "Placeholder text while waiting for font > settings to appear, shown only until font settings have loaded." (just a > suggestion) Done.
On 2015/10/06 00:10:32, Dan Beam wrote: > i don't see any titlecase in old IDS_OPTIONS*, so we should probably only do > sentence case for now Makes sense. They should all be sentence case now.
lgtm
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chantellb30@gmail.com, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1380743003/#ps120001 (title: "changed case of page title")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1380743003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1380743003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b43479fa6ac8e7f54871af30a78b66b0e3fca2a4 Cr-Commit-Position: refs/heads/master@{#353451} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
