Issue 6553361117609984: Issue 1816 - Add download Maxthon notification for Chinese locales (Website) (Closed)
|
Created:
Jan. 30, 2015, 11:26 a.m. by saroyanm Modified:
Feb. 10, 2015, 2:26 p.m. Reviewers:
Thomas Greiner Visibility:
Public. |
DescriptionInfrastructure changes:
http://codereview.adblockplus.org/4907991358767104/
Related ticket:
https://issues.adblockplus.org/ticket/1816
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Patch Set 5 : #MessagesTotal messages: 10
@Thomas please have a look when you will have time.
On 2015/01/30 11:47:35, saroyanm wrote: > @Thomas please have a look when you will have time. This review is ready to be reviewed after.
http://codereview.adblockplus.org/6553361117609984/diff/5741031244955648/inde... File index.html (right): http://codereview.adblockplus.org/6553361117609984/diff/5741031244955648/inde... index.html:74: <div class="maxthon-notification"> I doubt that we'll have multiple Maxthon notifications so let's use an ID here instead of a class.
http://codereview.adblockplus.org/6553361117609984/diff/5741031244955648/inde... File index.html (right): http://codereview.adblockplus.org/6553361117609984/diff/5741031244955648/inde... index.html:74: <div class="maxthon-notification"> On 2015/02/09 10:54:20, Thomas Greiner wrote: > I doubt that we'll have multiple Maxthon notifications so let's use an ID here > instead of a class. Done.
http://codereview.adblockplus.org/6553361117609984/diff/5717271485874176/inde... File index.html (right): http://codereview.adblockplus.org/6553361117609984/diff/5717271485874176/inde... index.html:29: Integrated by default in <a target="_blank"><attr name="href">http://www.maxthon.com/</attr>Maxthon Cloud Browser</a> No need to translate the URL so the <attr> tag is unnecessary here.
http://codereview.adblockplus.org/6553361117609984/diff/5717271485874176/inde... File index.html (right): http://codereview.adblockplus.org/6553361117609984/diff/5717271485874176/inde... index.html:29: Integrated by default in <a target="_blank"><attr name="href">http://www.maxthon.com/</attr>Maxthon Cloud Browser</a> On 2015/02/09 12:58:27, Thomas Greiner wrote: > No need to translate the URL so the <attr> tag is unnecessary here. We need to have different href attribute for different locales, ex.: Chinese -> maxthon.cn while English -> maxthon.com
http://codereview.adblockplus.org/6553361117609984/diff/5717271485874176/inde... File index.html (right): http://codereview.adblockplus.org/6553361117609984/diff/5717271485874176/inde... index.html:29: Integrated by default in <a target="_blank"><attr name="href">http://www.maxthon.com/</attr>Maxthon Cloud Browser</a> On 2015/02/09 13:01:03, saroyanm wrote: > On 2015/02/09 12:58:27, Thomas Greiner wrote: > > No need to translate the URL so the <attr> tag is unnecessary here. > > We need to have different href attribute for different locales, > ex.: Chinese -> maxthon.cn while English -> http://maxthon.com Really? This was not mentioned in the issue description. Anyway, we should not allow translators to set links on our website or otherwise they might link to illegal sites or their own sites to improve their SEO. Either we use one single link or we show the appropriate link based on the locale (e.g. through CSS hiding).
http://codereview.adblockplus.org/6553361117609984/diff/5717271485874176/inde... File index.html (right): http://codereview.adblockplus.org/6553361117609984/diff/5717271485874176/inde... index.html:29: Integrated by default in <a target="_blank"><attr name="href">http://www.maxthon.com/</attr>Maxthon Cloud Browser</a> On 2015/02/09 14:07:11, Thomas Greiner wrote: > On 2015/02/09 13:01:03, saroyanm wrote: > > On 2015/02/09 12:58:27, Thomas Greiner wrote: > > > No need to translate the URL so the <attr> tag is unnecessary here. > > > > We need to have different href attribute for different locales, > > ex.: Chinese -> maxthon.cn while English -> http://maxthon.com > > Really? This was not mentioned in the issue description. Anyway, we should not > allow translators to set links on our website or otherwise they might link to > illegal sites or their own sites to improve their SEO. > > Either we use one single link or we show the appropriate link based on the > locale (e.g. through CSS hiding). Good point, haven't thought about that issue. Vicky discussed with Maxthon and they confirm that they have redirection for locales, so we can go with maxthon.com
On 2015/02/09 14:31:08, saroyanm wrote: > http://codereview.adblockplus.org/6553361117609984/diff/5717271485874176/inde... > File index.html (right): > > http://codereview.adblockplus.org/6553361117609984/diff/5717271485874176/inde... > index.html:29: Integrated by default in <a target="_blank"><attr > name="href">http://www.maxthon.com/</attr>Maxthon Cloud Browser</a> > On 2015/02/09 14:07:11, Thomas Greiner wrote: > > On 2015/02/09 13:01:03, saroyanm wrote: > > > On 2015/02/09 12:58:27, Thomas Greiner wrote: > > > > No need to translate the URL so the <attr> tag is unnecessary here. > > > > > > We need to have different href attribute for different locales, > > > ex.: Chinese -> maxthon.cn while English -> http://maxthon.com > > > > Really? This was not mentioned in the issue description. Anyway, we should not > > allow translators to set links on our website or otherwise they might link to > > illegal sites or their own sites to improve their SEO. > > > > Either we use one single link or we show the appropriate link based on the > > locale (e.g. through CSS hiding). > > Good point, haven't thought about that issue. > Vicky discussed with Maxthon and they confirm that they have redirection for > locales, so we can go with http://maxthon.com Small update, forgot about fixes :)
LGTM 
|
