Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2632633005:
Fix html attributes for RTL languages on the signin email confirmation dialog. (Closed)
DescriptionFix html attributes for RTL languages on the signin email confirmation dialog.
This CL fixes the direction and the languages attributes for RTL languages
on the signin email confirmation dialog.
BUG=674826
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2632633005
Cr-Commit-Position: refs/heads/master@{#445038}
Committed: https://chromium.googlesource.com/chromium/src/+/dcec08a8f2113594a47067527d4b326aca99ff47
Patch Set 1 #
Total comments: 3
Patch Set 2 : Address code review #Patch Set 3 : Rebase #Messages
Total messages: 24 (13 generated)
Description was changed from ========== Process i18n values on the signin email confirmation dialog. This CL runs includes the script js/i18n_template.js to process the i18n values used in the signin email confirmation dialog. BUG=674826 ========== to ========== Process i18n values on the signin email confirmation dialog. This CL runs includes the script js/i18n_template.js to process the i18n values used in the signin email confirmation dialog. BUG=674826 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Process i18n values on the signin email confirmation dialog. This CL runs includes the script js/i18n_template.js to process the i18n values used in the signin email confirmation dialog. BUG=674826 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Process i18n values on the signin email confirmation dialog. This CL runs includes the script js/i18n_template.js to process the i18n values used in the signin email confirmation dialog. BUG=674826 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
msarda@chromium.org changed reviewers: + dbeam@chromium.org
Please take a look.
https://codereview.chromium.org/2632633005/diff/1/chrome/browser/resources/si... File chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html (right): https://codereview.chromium.org/2632633005/diff/1/chrome/browser/resources/si... chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:2: <html i18n-values="dir:textdirection;lang:language"> does changing this to <html dir="$i18n{textdirection}" lang="$i18n{language}"> work instead?
https://codereview.chromium.org/2632633005/diff/1/chrome/browser/resources/si... File chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html (right): https://codereview.chromium.org/2632633005/diff/1/chrome/browser/resources/si... chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:2: <html i18n-values="dir:textdirection;lang:language"> On 2017/01/13 17:13:14, Dan Beam wrote: > does changing this to > > <html dir="$i18n{textdirection}" lang="$i18n{language}"> > > work instead? I have not tested it - I would suppose it does as it is used elsewhere in the code (see https://cs.chromium.org/search/?q=%22html+dir%3D%5C%22$i18n%7Btextdirection%7...) I thought it would be better to use the script for consistency - this is what is used in the sync_confirmation dialog which is part of the same sign-in flow - see https://cs.chromium.org/chromium/src/chrome/browser/resources/signin/sync_con... What is the preferred way - including the script i18n_template or using your proposal (<html dir="$i18n{textdirection}" lang="$i18n{language}">)?
On 2017/01/13 19:53:13, msarda wrote: > https://codereview.chromium.org/2632633005/diff/1/chrome/browser/resources/si... > File > chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html > (right): > > https://codereview.chromium.org/2632633005/diff/1/chrome/browser/resources/si... > chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:2: > <html i18n-values="dir:textdirection;lang:language"> > On 2017/01/13 17:13:14, Dan Beam wrote: > > does changing this to > > > > <html dir="$i18n{textdirection}" lang="$i18n{language}"> > > > > work instead? > > I have not tested it - I would suppose it does as it is used elsewhere in the > code (see > https://cs.chromium.org/search/?q=%22html+dir%3D%5C%22$i18n%7Btextdirection%7...) > > I thought it would be better to use the script for consistency - this is what is > used in the sync_confirmation dialog which is part of the same sign-in flow - > see > https://cs.chromium.org/chromium/src/chrome/browser/resources/signin/sync_con... > > What is the preferred way - including the script i18n_template or using your > proposal (<html dir="$i18n{textdirection}" lang="$i18n{language}">)? my proposal is faster, does not flicker or require scripts, and was invented to destroy i18n_template so... mine :)
https://codereview.chromium.org/2632633005/diff/1/chrome/browser/resources/si... File chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html (right): https://codereview.chromium.org/2632633005/diff/1/chrome/browser/resources/si... chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:2: <html i18n-values="dir:textdirection;lang:language"> On 2017/01/13 19:53:13, msarda wrote: > On 2017/01/13 17:13:14, Dan Beam wrote: > > does changing this to > > > > <html dir="$i18n{textdirection}" lang="$i18n{language}"> > > > > work instead? > > I have not tested it - I would suppose it does as it is used elsewhere in the > code (see > https://cs.chromium.org/search/?q=%22html+dir%3D%5C%22$i18n%7Btextdirection%7...) > > I thought it would be better to use the script for consistency - this is what is > used in the sync_confirmation dialog which is part of the same sign-in flow - > see > https://cs.chromium.org/chromium/src/chrome/browser/resources/signin/sync_con... > > What is the preferred way - including the script i18n_template or using your > proposal (<html dir="$i18n{textdirection}" lang="$i18n{language}">)? Done.
lgtm but please update the CL description
Description was changed from ========== Process i18n values on the signin email confirmation dialog. This CL runs includes the script js/i18n_template.js to process the i18n values used in the signin email confirmation dialog. BUG=674826 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix dir and lang attributes for RTL languages on the signin email confirmation dialog. BUG=674826 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix dir and lang attributes for RTL languages on the signin email confirmation dialog. BUG=674826 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix html attributes for RTL languages on the signin email confirmation dialog. This CL fixes the direction and languages attributes on the signin email confirmation dialog. BUG=674826 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix html attributes for RTL languages on the signin email confirmation dialog. This CL fixes the direction and languages attributes on the signin email confirmation dialog. BUG=674826 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix html attributes for RTL languages on the signin email confirmation dialog. This CL fixes the direction and languages attributes for RTL languages on the signin email confirmation dialog. BUG=674826 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix html attributes for RTL languages on the signin email confirmation dialog. This CL fixes the direction and languages attributes for RTL languages on the signin email confirmation dialog. BUG=674826 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix html attributes for RTL languages on the signin email confirmation dialog. This CL fixes the direction and the languages attributes for RTL languages on the signin email confirmation dialog. BUG=674826 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
I fixed the CL description (thank you for pointing that out).
The CQ bit was checked by msarda@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
Failed to apply patch for
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:
While running git apply --index -p1;
error: patch failed:
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:1
error:
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html:
patch does not apply
Patch:
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html
Index:
chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html
diff --git
a/chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html
b/chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html
index
c81130284be54ce4cdfb5aa0215e3d92b69421c8..ed05a350f802d19fe9fe22815adcee15bf95b5b0
100644
---
a/chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html
+++
b/chrome/browser/resources/signin/signin_email_confirmation/signin_email_confirmation.html
@@ -1,5 +1,5 @@
<!doctype html>
-<html i18n-values="dir:textdirection;lang:language">
+<html dir="$i18n{textdirection}" lang="$i18n{language}">
<head>
<meta charset="utf-8">
<link rel="import" href="chrome://resources/html/polymer.html">
The CQ bit was checked by msarda@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2632633005/#ps40001 (title: "Rebase")
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": 1484905139862230,
"parent_rev": "0893b8df9552dd6d4d8828e3a4332ed2ce6a9c77", "commit_rev":
"dcec08a8f2113594a47067527d4b326aca99ff47"}
Message was sent while issue was closed.
Description was changed from ========== Fix html attributes for RTL languages on the signin email confirmation dialog. This CL fixes the direction and the languages attributes for RTL languages on the signin email confirmation dialog. BUG=674826 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix html attributes for RTL languages on the signin email confirmation dialog. This CL fixes the direction and the languages attributes for RTL languages on the signin email confirmation dialog. BUG=674826 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2632633005 Cr-Commit-Position: refs/heads/master@{#445038} Committed: https://chromium.googlesource.com/chromium/src/+/dcec08a8f2113594a47067527d4b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/dcec08a8f2113594a47067527d4b... |
