Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12039045:
History: Fix RTL layout in grouped history and some other minor fixes. (Closed)
|
Created:
7 years, 11 months ago by Sergiu Modified:
7 years, 10 months ago Reviewers:
Patrick Dubroy CC:
chromium-reviews, Patrick Dubroy, arv (Not doing code reviews), pam+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix RTL layout in grouped history and some other minor fixes.
Grouped results now show properly in RTL direction. Also added a space between
the domain and the number of visits and fixed some minor code style issues.
BUG=170690
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181490
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase #Patch Set 3 : More fixes #
Total comments: 13
Patch Set 4 : Rebase #Patch Set 5 : More fixes #Patch Set 6 : Rebase #Patch Set 7 : Few more fixes after the latest history patch. #
Total comments: 2
Patch Set 8 : Last minor fixes #
Messages
Total messages: 12 (0 generated)
Please take a look. Thanks, Sergiu
https://codereview.chromium.org/12039045/diff/1/chrome/browser/resources/hist... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/12039045/diff/1/chrome/browser/resources/hist... chrome/browser/resources/history/history.js:380: // If the results are not for the current search term then there is nothing Didn't want to do what James suggested? ;-) https://codereview.chromium.org/12039045/diff/1/chrome/browser/resources/hist... chrome/browser/resources/history/history.js:723: siteDomain.textContent = ' ' + domain + ' '; Why are you adding space to both sides of the domain here? If it's to add space before the parentheses and have it work in RTL, I think there are better ways. Either use -webkit-margin-start or -webkit-margin-end and set a margin of 1em, or put inside the span.
Fixed the domain regex, added a span to the domain and fixed some more minor things. https://codereview.chromium.org/12039045/diff/1/chrome/browser/resources/hist... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/12039045/diff/1/chrome/browser/resources/hist... chrome/browser/resources/history/history.js:380: // If the results are not for the current search term then there is nothing On 2013/01/25 15:29:59, dubroy wrote: > Didn't want to do what James suggested? ;-) I realized that I didn't like the sentence with or without the comma and found it better with "then" in it :) https://codereview.chromium.org/12039045/diff/1/chrome/browser/resources/hist... chrome/browser/resources/history/history.js:723: siteDomain.textContent = ' ' + domain + ' '; On 2013/01/25 15:29:59, dubroy wrote: > Why are you adding space to both sides of the domain here? If it's to add space > before the parentheses and have it work in RTL, I think there are better ways. > Either use -webkit-margin-start or -webkit-margin-end and set a margin of 1em, > or put inside the span. Put in a -webkit-margin-end but it may still not work (aka have the space) properly on rtl languages. I'll test some more.
https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... chrome/browser/resources/history/history.css:128: clear: right; Perhaps just use "clear: both" for simplicity? https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... chrome/browser/resources/history/history.css:165: /* TODO(sergiu): make sure this works on rtl properly */ You should remove this TODO before landing...you are going to test this before landing, right? https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... chrome/browser/resources/history/history.css:282: -webkit-transform: scaleX(-1); Maybe just do this with rotate() too? Might be a bit simpler that way. https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... chrome/browser/resources/history/history.js:162: var domain = url.replace(/^.+?:\/\//, '').match(/[^/]+/); And...this is why doing this is with a regex is a bad idea. :-) We should probably just parse the domain in C++ and then pass it in along with the rest of the visit info. We can do that in another CL though. https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... chrome/browser/resources/history/history.js:971: if (grouped) I'd change this to: $('display-filter-sites').checked = grouped;
These are some comments I had, I'll take care of the rest tomorrow. https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... chrome/browser/resources/history/history.css:165: /* TODO(sergiu): make sure this works on rtl properly */ On 2013/01/30 19:23:04, dubroy wrote: > You should remove this TODO before landing...you are going to test this before > landing, right? Well, it works in the current version, but I'm not sure about one thing, is there any function that transforms numbers to locale specific versions? I imagine that numbers are represented differently in some languages. Also the parentheses seem like a pretty big complication from that point of view, I'm just tempted to switch to something like "www.google.com - 5 visits" instead of "www.google.com (5)" https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... chrome/browser/resources/history/history.js:162: var domain = url.replace(/^.+?:\/\//, '').match(/[^/]+/); On 2013/01/30 19:23:04, dubroy wrote: > And...this is why doing this is with a regex is a bad idea. :-) > > We should probably just parse the domain in C++ and then pass it in along with > the rest of the visit info. We can do that in another CL though. I'll put in a TODO.
https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... chrome/browser/resources/history/history.css:165: /* TODO(sergiu): make sure this works on rtl properly */ On 2013/01/30 21:12:43, Sergiu wrote: > On 2013/01/30 19:23:04, dubroy wrote: > > You should remove this TODO before landing...you are going to test this before > > landing, right? > > Well, it works in the current version, but I'm not sure about one thing, is > there any function that transforms numbers to locale specific versions? I > imagine that numbers are represented differently in some languages. Also the > parentheses seem like a pretty big complication from that point of view, I'm > just tempted to switch to something like "www.google.com - 5 visits" instead of > "www.google.com (5)" I think arabic numerals are understood in most locales, and I think there are many places in Chrome where we display numbers without localizing them, so I think it's ok.
I've tested it on Arabic and it looks good now (visually at least). https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... chrome/browser/resources/history/history.css:128: clear: right; On 2013/01/30 19:23:04, dubroy wrote: > Perhaps just use "clear: both" for simplicity? Done. https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... chrome/browser/resources/history/history.css:165: /* TODO(sergiu): make sure this works on rtl properly */ On 2013/02/02 00:36:24, dubroy wrote: > On 2013/01/30 21:12:43, Sergiu wrote: > > On 2013/01/30 19:23:04, dubroy wrote: > > > You should remove this TODO before landing...you are going to test this > before > > > landing, right? > > > > Well, it works in the current version, but I'm not sure about one thing, is > > there any function that transforms numbers to locale specific versions? I > > imagine that numbers are represented differently in some languages. Also the > > parentheses seem like a pretty big complication from that point of view, I'm > > just tempted to switch to something like "www.google.com - 5 visits" instead > of > > "www.google.com (5)" > > I think arabic numerals are understood in most locales, and I think there are > many places in Chrome where we display numbers without localizing them, so I > think it's ok. I've tested it in Arabic now as well and there's a single problem: the parentheses mess things up. They trigger the engine to think that the construct is always in LTR, which makes using things like -webkit-margin-end impossible. I've used margins and dir='rtl' in the end. https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... chrome/browser/resources/history/history.css:282: -webkit-transform: scaleX(-1); On 2013/01/30 19:23:04, dubroy wrote: > Maybe just do this with rotate() too? Might be a bit simpler that way. Done. https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... File chrome/browser/resources/history/history.js (right): https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... chrome/browser/resources/history/history.js:162: var domain = url.replace(/^.+?:\/\//, '').match(/[^/]+/); On 2013/01/30 21:12:43, Sergiu wrote: > On 2013/01/30 19:23:04, dubroy wrote: > > And...this is why doing this is with a regex is a bad idea. :-) > > > > We should probably just parse the domain in C++ and then pass it in along with > > the rest of the visit info. We can do that in another CL though. > > I'll put in a TODO. Put the TODO in. https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... chrome/browser/resources/history/history.js:971: if (grouped) On 2013/01/30 19:23:04, dubroy wrote: > I'd change this to: > > $('display-filter-sites').checked = grouped; Done.
Ping here as well :) On 2013/02/05 18:21:22, Sergiu wrote: > I've tested it on Arabic and it looks good now (visually at least). > > https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... > File chrome/browser/resources/history/history.css (right): > > https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... > chrome/browser/resources/history/history.css:128: clear: right; > On 2013/01/30 19:23:04, dubroy wrote: > > Perhaps just use "clear: both" for simplicity? > > Done. > > https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... > chrome/browser/resources/history/history.css:165: /* TODO(sergiu): make sure > this works on rtl properly */ > On 2013/02/02 00:36:24, dubroy wrote: > > On 2013/01/30 21:12:43, Sergiu wrote: > > > On 2013/01/30 19:23:04, dubroy wrote: > > > > You should remove this TODO before landing...you are going to test this > > before > > > > landing, right? > > > > > > Well, it works in the current version, but I'm not sure about one thing, is > > > there any function that transforms numbers to locale specific versions? I > > > imagine that numbers are represented differently in some languages. Also the > > > parentheses seem like a pretty big complication from that point of view, I'm > > > just tempted to switch to something like "www.google.com - 5 visits" instead > > of > > > "www.google.com (5)" > > > > I think arabic numerals are understood in most locales, and I think there are > > many places in Chrome where we display numbers without localizing them, so I > > think it's ok. > > I've tested it in Arabic now as well and there's a single problem: the > parentheses mess things up. They trigger the engine to think that the construct > is always in LTR, which makes using things like -webkit-margin-end impossible. > I've used margins and dir='rtl' in the end. > > https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... > chrome/browser/resources/history/history.css:282: -webkit-transform: scaleX(-1); > On 2013/01/30 19:23:04, dubroy wrote: > > Maybe just do this with rotate() too? Might be a bit simpler that way. > > Done. > > https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... > File chrome/browser/resources/history/history.js (right): > > https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... > chrome/browser/resources/history/history.js:162: var domain = > url.replace(/^.+?:\/\//, '').match(/[^/]+/); > On 2013/01/30 21:12:43, Sergiu wrote: > > On 2013/01/30 19:23:04, dubroy wrote: > > > And...this is why doing this is with a regex is a bad idea. :-) > > > > > > We should probably just parse the domain in C++ and then pass it in along > with > > > the rest of the visit info. We can do that in another CL though. > > > > I'll put in a TODO. > Put the TODO in. > > https://codereview.chromium.org/12039045/diff/7001/chrome/browser/resources/h... > chrome/browser/resources/history/history.js:971: if (grouped) > On 2013/01/30 19:23:04, dubroy wrote: > > I'd change this to: > > > > $('display-filter-sites').checked = grouped; > > Done.
lgtm. Sorry for the delay in reviewing this. https://codereview.chromium.org/12039045/diff/17006/chrome/browser/resources/... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/12039045/diff/17006/chrome/browser/resources/... chrome/browser/resources/history/history.css:71: -webkit-padding-start: 6px; Can't this just be "padding: 6px"?
https://codereview.chromium.org/12039045/diff/17006/chrome/browser/resources/... File chrome/browser/resources/history/history.css (right): https://codereview.chromium.org/12039045/diff/17006/chrome/browser/resources/... chrome/browser/resources/history/history.css:71: -webkit-padding-start: 6px; On 2013/02/07 19:17:22, dubroy wrote: > Can't this just be "padding: 6px"? That would add padding to the top and bottom as well, don't want that. Replaced it with padding: 0 6px 0 px;
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergiu@chromium.org/12039045/16005
Message was sent while issue was closed.
Change committed as 181490 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
