Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(232)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2563143002: MD Settings: About: Fixes to channel switcher dialog (Closed)

Created:
4 years ago by stevenjb
Modified:
4 years ago
Reviewers:
Dan Beam, 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/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

MD Settings: About: Fixes to channel switcher dialog BUG=666876 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/fa492598d6c2db6b6666e8f0f95c970d41c48e1c Cr-Commit-Position: refs/heads/master@{#438038}

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -4 lines) Patch
M chrome/app/settings_strings.grdp View 1 chunk +9 lines, -0 lines 0 comments Download
M chrome/browser/resources/settings/about_page/channel_switcher_dialog.html View 2 chunks +8 lines, -4 lines 2 comments Download
M chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc View 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 23 (10 generated)
stevenjb
4 years ago (2016-12-09 22:02:06 UTC) #3
michaelpg
lgtm wrong CL bug #?
4 years ago (2016-12-12 20:58:27 UTC) #4
stevenjb
Oops, thanks, fixed.
4 years ago (2016-12-12 21:04:35 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2563143002/1
4 years ago (2016-12-12 21:06:12 UTC) #8
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/278166)
4 years ago (2016-12-13 01:53:30 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2563143002/1
4 years ago (2016-12-13 02:07:09 UTC) #12
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years ago (2016-12-13 03:44:18 UTC) #15
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/fa492598d6c2db6b6666e8f0f95c970d41c48e1c Cr-Commit-Position: refs/heads/master@{#438038}
4 years ago (2016-12-13 03:46:33 UTC) #17
Dan Beam
https://codereview.chromium.org/2563143002/diff/1/chrome/browser/resources/settings/about_page/channel_switcher_dialog.html File chrome/browser/resources/settings/about_page/channel_switcher_dialog.html (left): https://codereview.chromium.org/2563143002/diff/1/chrome/browser/resources/settings/about_page/channel_switcher_dialog.html#oldcode28 chrome/browser/resources/settings/about_page/channel_switcher_dialog.html:28: $i18n{aboutChannelDev} so wait... we want the new strings to ...
4 years ago (2016-12-14 03:41:02 UTC) #19
stevenjb
https://codereview.chromium.org/2563143002/diff/1/chrome/browser/resources/settings/about_page/channel_switcher_dialog.html File chrome/browser/resources/settings/about_page/channel_switcher_dialog.html (left): https://codereview.chromium.org/2563143002/diff/1/chrome/browser/resources/settings/about_page/channel_switcher_dialog.html#oldcode28 chrome/browser/resources/settings/about_page/channel_switcher_dialog.html:28: $i18n{aboutChannelDev} On 2016/12/14 03:41:02, Dan Beam wrote: > so ...
4 years ago (2016-12-14 19:42:37 UTC) #20
Dan Beam
On 2016/12/14 19:42:37, stevenjb wrote: > https://codereview.chromium.org/2563143002/diff/1/chrome/browser/resources/settings/about_page/channel_switcher_dialog.html > File chrome/browser/resources/settings/about_page/channel_switcher_dialog.html > (left): > > https://codereview.chromium.org/2563143002/diff/1/chrome/browser/resources/settings/about_page/channel_switcher_dialog.html#oldcode28 ...
4 years ago (2016-12-14 22:44:04 UTC) #21
stevenjb
I'll double check with Alan about the capitalization. I interpreted the channel labels here (by ...
4 years ago (2016-12-14 22:50:58 UTC) #22
stevenjb
4 years ago (2016-12-14 23:16:07 UTC) #23
Message was sent while issue was closed. 
On 2016/12/14 22:50:58, stevenjb wrote:
> I'll double check with Alan about the capitalization. I interpreted the
> channel labels here (by themselves) as names. In any case, the context is
> different, which may affect localization (stand alone vs embedded in a
> sentence).
> 
> 
> On Dec 14, 2016 2:44 PM, <mailto:dbeam@chromium.org> wrote:
> 
> > On 2016/12/14 19:42:37, stevenjb wrote:
> > >
> > https://codereview.chromium.org/2563143002/diff/1/chrome/
> > browser/resources/settings/about_page/channel_switcher_dialog.html
> > > File chrome/browser/resources/settings/about_page/channel_
> > switcher_dialog.html
> > > (left):
> > >
> > >
> > https://codereview.chromium.org/2563143002/diff/1/chrome/
> > browser/resources/settings/about_page/channel_switcher_
> > dialog.html#oldcode28
> > > chrome/browser/resources/settings/about_page/channel_
> > switcher_dialog.html:28:
> > > $i18n{aboutChannelDev}
> > > On 2016/12/14 03:41:02, Dan Beam wrote:
> > > > so wait... we want the new strings to differ from other parts of the
> > code?
> > > >
> > > > settings.browserChannelToI18nId uses the old I18n keys:
> > > >
> > >
> > https://cs.chromium.org/chromium/src/chrome/browser/
> > resources/settings/about_page/about_page.js?q=browserChannelToI18nId&sq=
> > package:chromium&l=222&dr=C
> > > >
> > > > which is called by getUpdateStatusMessage_
> > > >
> > >
> > https://cs.chromium.org/chromium/src/chrome/browser/
> > resources/settings/about_page/about_page.html?q=
> > getUpdateStatusMessage_&sq=package:chromium&l=94&dr=C
> > > >
> > > > do we want these UIs to match?
> > >
> > > Those strings are inserted into a sentence:
> > >
> > >
> > https://cs.chromium.org/chromium/src/chrome/app/
> > settings_strings.grdp?q=IDS_SETTINGS_UPGRADE_UPDATING_
> > CHANNEL_SWITCH&sq=package:chromium&l=54&dr=C
> > >
> > > We were being a bit lazy here and just using the lower case "stable",
> > etc in
> > the
> > > dropdown also, instead of capitalized strings (with some extra text for
> > dev):
> > >
> > >
> > https://cs.chromium.org/chromium/src/chrome/app/
> > settings_strings.grdp?q=IDS_SETTINGS_ABOUT_PAGE_DIALOG_
> > CHANNEL_DEV&sq=package:chromium&dr=C&l=96
> >
> > fwiw: material design has opinions about title case and use of
> > punctuations, so
> > what you may interpret as "lazy" may actually be desired by the spec
> >
> > https://material.google.com/style/writing.html#writing-
> > capitalization-punctuation
> >
> > https://codereview.chromium.org/2563143002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Also, the spec says specifically:

"Use sentence-style caps for all titles, headings, labels, menu items"

So, it seems to me that the bug was correct and that these should be
capitalized. That is also consistent with all of the Settings menus that I have
tried.

Powered by Google App Engine
This is Rietveld 408576698