samarth@chromium.org: Please review changes in search -- and general hotword consistency jhawkins@chromium.org: Please review changes ...
5 years, 11 months ago
(2014-01-14 00:20:59 UTC)
#1
samarth@chromium.org: Please review changes in search -- and general hotword
consistency
jhawkins@chromium.org: Please review changes in resources/, webui/, chrome/
Also, please let me know if there are any tests that are typically added for
these sorts of preferences.
Thanks!
search lgtm https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_resources.grd#newcode6912 chrome/app/generated_resources.grd:6912: + Enable "Ok Google" to start a ...
5 years, 11 months ago
(2014-01-17 01:26:40 UTC)
#4
search lgtm
https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):
https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6912: + Enable "Ok Google" to start a
voice search. <ph name="BEGIN_LINK"><a target="_blank"
href="$1"></ph>Learn more<ph
name="END_LINK"></a><ex></a></ex></ph>
I assume these string have already been approved?
https://codereview.chromium.org/134103005/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):
https://codereview.chromium.org/134103005/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:157: <span
i18n-content="hotwordSearchDescription"></span>
On 2014/01/14 21:54:18, rpetterson wrote:
> This line is actually supposed to be a light gray per the mocks. I've tried
> every combination of id, class, divs, spans, p's, etc. I can think of to get
it
> to listen to browser_options.css, but something keeps overriding it and making
> it solid black. Even doing something like:
>
> #hotword-search-description {
> color: #333;
> }
>
> doesn't work. Any ideas?
You're probably entering a "css specificity war". In short, you can probably
make your style take effect by adding a more specific rule (e.g.
#hotword-search-description span {}). Or adding a !important to the rule. But
it's probably worth figuring out if there's a better way to restructure the
markup/css so you don't need to do that.
5 years, 11 months ago
(2014-01-17 17:39:11 UTC)
#5
On 2014/01/17 01:26:40, samarth wrote:
> search lgtm
>
>
https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_re...
> File chrome/app/generated_resources.grd (right):
>
>
https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_re...
> chrome/app/generated_resources.grd:6912: + Enable "Ok Google" to start
a
> voice search. <ph name="BEGIN_LINK"><a target="_blank"
> href="$1"></ph>Learn more<ph
> name="END_LINK"></a><ex></a></ex></ph>
> I assume these string have already been approved?
>
>
https://codereview.chromium.org/134103005/diff/120001/chrome/browser/resource...
> File chrome/browser/resources/options/browser_options.html (right):
>
>
https://codereview.chromium.org/134103005/diff/120001/chrome/browser/resource...
> chrome/browser/resources/options/browser_options.html:157: <span
> i18n-content="hotwordSearchDescription"></span>
> On 2014/01/14 21:54:18, rpetterson wrote:
> > This line is actually supposed to be a light gray per the mocks. I've tried
> > every combination of id, class, divs, spans, p's, etc. I can think of to get
> it
> > to listen to browser_options.css, but something keeps overriding it and
making
> > it solid black. Even doing something like:
> >
> > #hotword-search-description {
> > color: #333;
> > }
> >
> > doesn't work. Any ideas?
>
> You're probably entering a "css specificity war". In short, you can probably
> make your style take effect by adding a more specific rule (e.g.
> #hotword-search-description span {}). Or adding a !important to the rule.
But
> it's probably worth figuring out if there's a better way to restructure the
> markup/css so you don't need to do that.
Beyond that I usually just check the inspector to see which specifier is
overriding this, then figure out how to beat it in the 'war' as samarth put it
:-)
James Hawkins
https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources/options/browser_options.css File chrome/browser/resources/options/browser_options.css (right): https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources/options/browser_options.css#newcode160 chrome/browser/resources/options/browser_options.css:160: margin-left: 22px; On 2014/01/14 21:54:17, rpetterson wrote: > On ...
5 years, 11 months ago
(2014-01-17 17:39:18 UTC)
#6
https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources/options/browser_options.css File chrome/browser/resources/options/browser_options.css (right): https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources/options/browser_options.css#newcode160 chrome/browser/resources/options/browser_options.css:160: margin-left: 22px; On 2014/01/17 17:39:18, James Hawkins wrote: > ...
5 years, 11 months ago
(2014-01-17 23:53:23 UTC)
#7
https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.css (right):
https://codereview.chromium.org/134103005/diff/50001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.css:160: margin-left: 22px;
On 2014/01/17 17:39:18, James Hawkins wrote:
> On 2014/01/14 21:54:17, rpetterson wrote:
> > On 2014/01/14 00:53:50, James Hawkins wrote:
> > > RTL
> >
> > What's the best way to handle this? Just "margin"?
>
> -webkit-margin-start
Done.
https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):
https://codereview.chromium.org/134103005/diff/120001/chrome/app/generated_re...
chrome/app/generated_resources.grd:6912: + Enable "Ok Google" to start a
voice search. <ph name="BEGIN_LINK"><a target="_blank"
href="$1"></ph>Learn more<ph
name="END_LINK"></a><ex></a></ex></ph>
On 2014/01/17 01:26:41, samarth wrote:
> I assume these string have already been approved?
They are directly from the mocks I was given, but they are also behind a flag.
My understanding is that part of getting them in 2 weeks before branch is to get
the string approval.
https://codereview.chromium.org/134103005/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.html (right):
https://codereview.chromium.org/134103005/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.html:157: <span
i18n-content="hotwordSearchDescription"></span>
On 2014/01/17 01:26:41, samarth wrote:
> On 2014/01/14 21:54:18, rpetterson wrote:
> > This line is actually supposed to be a light gray per the mocks. I've tried
> > every combination of id, class, divs, spans, p's, etc. I can think of to get
> it
> > to listen to browser_options.css, but something keeps overriding it and
making
> > it solid black. Even doing something like:
> >
> > #hotword-search-description {
> > color: #333;
> > }
> >
> > doesn't work. Any ideas?
>
> You're probably entering a "css specificity war". In short, you can probably
> make your style take effect by adding a more specific rule (e.g.
> #hotword-search-description span {}). Or adding a !important to the rule.
But
> it's probably worth figuring out if there's a better way to restructure the
> markup/css so you don't need to do that.
So I actually had this right before, but I just had the wrong color in there so
I couldn't see the effect. Thanks for the tip on the inspector. It was quite
helpful for figuring this out. I've updated the CSS.
James Hawkins
LGTM with nit. https://codereview.chromium.org/134103005/diff/200001/chrome/browser/resources/options/browser_options.css File chrome/browser/resources/options/browser_options.css (right): https://codereview.chromium.org/134103005/diff/200001/chrome/browser/resources/options/browser_options.css#newcode164 chrome/browser/resources/options/browser_options.css:164: color: #9B9B9B; nit: Lower-case letters in ...
5 years, 10 months ago
(2014-01-21 18:18:02 UTC)
#8
Issue 134103005: [Hotword] Putting preferences under search for hotword service. Putting behind a flag.
(Closed)
Created 5 years, 11 months ago by rpetterson
Modified 5 years, 10 months ago
Reviewers: samarth, James Hawkins
Base URL: http://git.chromium.org/chromium/src.git@master
Comments: 17