Issue 29430610: Issue 5193 - Apply Samsung Internet UI changes to the frist run slide

Issue 29430610: Issue 5193 - Apply Samsung Internet UI changes to the frist run slide (Closed)

Created:
May 5, 2017, 2:58 p.m. by jens

Modified:
May 10, 2017, 10:10 a.m.

Reviewers:
diegocarloslima

CC:
Felix Dahlke

Visibility:
Public.

Description

Issue 5193 - Apply Samsung Internet UI changes to the frist run slide

Patch Set 2 : Changes according to Diegos comments #

Created: May 9, 2017, 9:44 a.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+40 lines, -13 lines)			Patch
M	adblockplussbrowser/res/values/strings.xml	View	1	1 chunk	+5 lines, -4 lines	0 comments	Download
M	adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java	View	1	5 chunks	+5 lines, -9 lines	0 comments	Download
M	adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/engine/Engine.java	View	1	4 chunks	+30 lines, -0 lines	0 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

diegocarloslima

https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser/AndroidManifest.xml File adblockplussbrowser/AndroidManifest.xml (right): https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser/AndroidManifest.xml#newcode3 adblockplussbrowser/AndroidManifest.xml:3: package="org.adblockplus.adblockplussbrowser"> We still wanna have support for Eclipse, so ...

May 5, 2017, 5:44 p.m. (2017-05-05 17:44:11 UTC) #2

jens

May 9, 2017, 9:44 a.m. (2017-05-09 09:44:45 UTC) #3

https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
File adblockplussbrowser/AndroidManifest.xml (right):

https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
adblockplussbrowser/AndroidManifest.xml:3:
package="org.adblockplus.adblockplussbrowser">
On 2017/05/05 17:44:11, diegocarloslima wrote:
> We still wanna have support for Eclipse, so for now we shouldn't change this.
> Also, for changes that are unrelated to the current issue, we usually create a
> follow up ticket.

Acknowledged.

https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
File adblockplussbrowser/res/values/strings.xml (right):

https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
adblockplussbrowser/res/values/strings.xml:21: <string
name="aa_dialog_message"><![CDATA[<p>Annoying ads are always blocked, while
nonintrusive ads are displayed by default. You can change this setting at any
time by tapping the <b>Allow some nonintrusive ads</b> option.]]></string>
On 2017/05/05 17:44:11, diegocarloslima wrote:
> I don't feel the need for this <p> tag. Any particular reason for that?
(Anyways
> is missing closing tag </p>)
You're right, the <p> tag shouldn't be there.

https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
adblockplussbrowser/res/values/strings.xml:28: <string
name="setup_dialog_message_sbrowser_5"><![CDATA[1. Open Samsung Internet.<br/>2.
Tap &nbsp; &#8942; &nbsp; and select <b>Extensions</b>.<br/>3. Tap <b>Content
blockers</b>.<br/>4. Select Adblock Plus for Samsung Internet. ]]></string>
On 2017/05/05 17:44:11, diegocarloslima wrote:
> Why &nbsp; was used here? To avoid trimming the spaces? Also, from the ticket
> description, the vertical dots are bold.

I tried to make the vertical dots bold but I did not see any difference between
bold and not bold. 
I used &nbsp; for some space left and right of the vertical dots, because
without the spaces is looked a bit weird in my opinion.

https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
File
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java
(right):

https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:42:
public static final String SBROWSER_APP_ID = "com.sec.android.app.sbrowser";
On 2017/05/05 17:44:11, diegocarloslima wrote:
> I think it will be better to move this field to Engine class, once it is being
> used there. The common purpose field are usually there.

Acknowledged.

diegocarloslima

May 9, 2017, 2:21 p.m. (2017-05-09 14:21:25 UTC) #4

On 2017/05/09 09:44:45, jens wrote:
>
https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
> File adblockplussbrowser/AndroidManifest.xml (right):
> 
>
https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
> adblockplussbrowser/AndroidManifest.xml:3:
> package="org.adblockplus.adblockplussbrowser">
> On 2017/05/05 17:44:11, diegocarloslima wrote:
> > We still wanna have support for Eclipse, so for now we shouldn't change
this.
> > Also, for changes that are unrelated to the current issue, we usually create
a
> > follow up ticket.
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
> File adblockplussbrowser/res/values/strings.xml (right):
> 
>
https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
> adblockplussbrowser/res/values/strings.xml:21: <string
> name="aa_dialog_message"><![CDATA[<p>Annoying ads are always blocked, while
> nonintrusive ads are displayed by default. You can change this setting at any
> time by tapping the <b>Allow some nonintrusive ads</b> option.]]></string>
> On 2017/05/05 17:44:11, diegocarloslima wrote:
> > I don't feel the need for this <p> tag. Any particular reason for that?
> (Anyways
> > is missing closing tag </p>)
> You're right, the <p> tag shouldn't be there.
> 
>
https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
> adblockplussbrowser/res/values/strings.xml:28: <string
> name="setup_dialog_message_sbrowser_5"><![CDATA[1. Open Samsung
Internet.<br/>2.
> Tap &nbsp; &#8942; &nbsp; and select <b>Extensions</b>.<br/>3. Tap <b>Content
> blockers</b>.<br/>4. Select Adblock Plus for Samsung Internet. ]]></string>
> On 2017/05/05 17:44:11, diegocarloslima wrote:
> > Why &nbsp; was used here? To avoid trimming the spaces? Also, from the
ticket
> > description, the vertical dots are bold.
> 
> I tried to make the vertical dots bold but I did not see any difference
between
> bold and not bold. 
> I used &nbsp; for some space left and right of the vertical dots, because
> without the spaces is looked a bit weird in my opinion.
> 
>
https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
> File
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java
> (right):
> 
>
https://codereview.adblockplus.org/29430610/diff/29430611/adblockplussbrowser...
>
adblockplussbrowser/src/org/adblockplus/sbrowser/contentblocker/MainPreferences.java:42:
> public static final String SBROWSER_APP_ID = "com.sec.android.app.sbrowser";
> On 2017/05/05 17:44:11, diegocarloslima wrote:
> > I think it will be better to move this field to Engine class, once it is
being
> > used there. The common purpose field are usually there.
> 
> Acknowledged.

LGTM

Expand All Messages | Collapse All Messages