Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2088913002:
Add strings needed for the PIN unlock section in md-settings. (Closed)
|
Created:
4 years, 6 months ago by jdufault Modified:
4 years, 6 months ago Reviewers:
tommycli 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@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd strings needed for the PIN unlock section in md-settings.
BUG=603217
Committed: https://crrev.com/7b28fe170d2d32c617621e88f0b89da8dd0e111d
Cr-Commit-Position: refs/heads/master@{#401654}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Update strings #Patch Set 3 : Make adjustments to cc bindings as well #Patch Set 4 : Rebase #Patch Set 5 : Remove dependency #Patch Set 6 : Remove PS dep, try 2 #
Messages
Total messages: 22 (13 generated)
Description was changed from ========== Add strings needed for the PIN unlock section in md-settings. BUG=603217 ========== to ========== Add strings needed for the PIN unlock section in md-settings. BUG=603217 ==========
jdufault@chromium.org changed reviewers: + tommycli@chromium.org
Tommy, PTAL. This patch includes just the needed strings.
https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:1153: <message name="IDS_SETTINGS_QUICK_UNLOCK_PAGE_TITLE" desc="The title of the quick unlock subpage."> These two messages look identical, and should just be consolidated into IDS_SETTINGS_PEOPLE_QUICK_UNLOCK (need a PEOPLE qualifier, drop the TITLE ending) https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:1156: <message name="IDS_SETTINGS_QUICK_UNLOCK_CONFIRM_LOGIN" desc="Text above a password input field that tells the user they need to submit their password to configure these settings."> In fact all of these should probably be IDS_SETTINGS_PEOPLE_QUICK_UNLOCK https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:1175: None This is fine for this CL, but "None" seems to be a confusing UI. It makes it seem like there's no method to unlock. Does "No lock" seem better? Maybe follow up with your designer. https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:1178: Disclaimer about how PIN is less secure than password. If the text isn't actually ready, I'd rather not put this in the list of strings, and instead add an english-only TODO in the HTML code. https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:1180: <message name="IDS_SETTINGS_QUICK_UNLOCK_CONFIGURE_PIN_BUTTON" desc="Button that is only shown when the user has selected PIN as their unlock method. This button opens up the configuration flow where the user selects the PIN they would like to use."> These descriptions of the messages are awesome. Makes reviewing this much easier. https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:1196: PIN's don't match: Should be a period at the end right? Also does there need to be a "Pin must be at least 4 digits" message? https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:1202: Save "Save" is already in the "common strings" section (see IDS_SAVE).
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:1153: <message name="IDS_SETTINGS_QUICK_UNLOCK_PAGE_TITLE" desc="The title of the quick unlock subpage."> On 2016/06/21 20:26:06, tommycli wrote: > These two messages look identical, and should just be consolidated into > IDS_SETTINGS_PEOPLE_QUICK_UNLOCK (need a PEOPLE qualifier, drop the TITLE > ending) Done. https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:1156: <message name="IDS_SETTINGS_QUICK_UNLOCK_CONFIRM_LOGIN" desc="Text above a password input field that tells the user they need to submit their password to configure these settings."> On 2016/06/21 20:26:06, tommycli wrote: > In fact all of these should probably be IDS_SETTINGS_PEOPLE_QUICK_UNLOCK Done. https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:1175: None On 2016/06/21 20:26:06, tommycli wrote: > This is fine for this CL, but "None" seems to be a confusing UI. It makes it > seem like there's no method to unlock. > > Does "No lock" seem better? Maybe follow up with your designer. Leaving this for now, we can adjust strings later when more people have a chance to see the design. We still have plenty of time to make minor adjustments to this UI. https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:1178: Disclaimer about how PIN is less secure than password. On 2016/06/21 20:26:06, tommycli wrote: > If the text isn't actually ready, I'd rather not put this in the list of > strings, and instead add an english-only TODO in the HTML code. That sounds reasonable. https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:1196: PIN's don't match: On 2016/06/21 20:26:06, tommycli wrote: > Should be a period at the end right? The problem messages seem to all end in ':'. > Also does there need to be a "Pin must be at least 4 digits" message? Ah! Good catch! I need to implement this. https://codereview.chromium.org/2088913002/diff/1/chrome/app/settings_strings... chrome/app/settings_strings.grdp:1202: Save On 2016/06/21 20:26:06, tommycli wrote: > "Save" is already in the "common strings" section (see IDS_SAVE). Done.
This is lgtm, though it looks like you will need to do a merge.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2088913002/#ps80001 (title: "Rebase")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1968083004 Patch 320001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2088913002/#ps100001 (title: "Remove dependency")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1968083004 Patch 320001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2088913002/#ps120001 (title: "Remove PS dep, try 2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2088913002/120001
Message was sent while issue was closed.
Description was changed from ========== Add strings needed for the PIN unlock section in md-settings. BUG=603217 ========== to ========== Add strings needed for the PIN unlock section in md-settings. BUG=603217 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add strings needed for the PIN unlock section in md-settings. BUG=603217 ========== to ========== Add strings needed for the PIN unlock section in md-settings. BUG=603217 Committed: https://crrev.com/7b28fe170d2d32c617621e88f0b89da8dd0e111d Cr-Commit-Position: refs/heads/master@{#401654} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7b28fe170d2d32c617621e88f0b89da8dd0e111d Cr-Commit-Position: refs/heads/master@{#401654} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
