Issue 5613 - Removal of social media: Share this number

Issue #5613 - Removal of social media: Share this number

[a.giammarchi, 2018-02-08 12:27:04]

As mentioned further down the #5613 ticket, we don't want to show social media
buttons with related share action.

[saroyanm, 2018-02-14 17:19:47]

https://codereview.adblockplus.org/29692555/diff/29692556/popup.html
File popup.html (left):

https://codereview.adblockplus.org/29692555/diff/29692556/popup.html#oldcode86
popup.html:86: <span class="i18n_stats_share_title"></span>
We still have this String in the "locales".
Removing it only from the en_US should be sufficient, translation script should
take care of removing it for the other locales.

https://codereview.adblockplus.org/29692555/diff/29692556/skin/popup.css
File skin/popup.css (left):

https://codereview.adblockplus.org/29692555/diff/29692556/skin/popup.css#oldc...
skin/popup.css:296: background-image: url(popup.png);
Detail/Nit: We still have the images in the popup.png file.

While it will require adjustment of the icons if we crop the image, I will
suggest to only remove the icons from the file keeping a transparent place.

[a.giammarchi, 2018-02-15 09:43:10]

one answer + one question (extra clarification)

https://codereview.adblockplus.org/29692555/diff/29692556/popup.html
File popup.html (left):

https://codereview.adblockplus.org/29692555/diff/29692556/popup.html#oldcode86
popup.html:86: <span class="i18n_stats_share_title"></span>
On 2018/02/14 17:19:46, saroyanm wrote:
> We still have this String in the "locales".
> Removing it only from the en_US should be sufficient, translation script
should
> take care of removing it for the other locales.

This is a different repository. I can do that only as follow up / another
ticket.

https://codereview.adblockplus.org/29692555/diff/29692556/skin/popup.css
File skin/popup.css (left):

https://codereview.adblockplus.org/29692555/diff/29692556/skin/popup.css#oldc...
skin/popup.css:296: background-image: url(popup.png);
On 2018/02/14 17:19:47, saroyanm wrote:
> Detail/Nit: We still have the images in the popup.png file.
> 
> While it will require adjustment of the icons if we crop the image, I will
> suggest to only remove the icons from the file keeping a transparent place.

Apologies I don't understand this comment or what should I do exactly.
Please expand a little bit more, thanks.

[saroyanm, 2018-02-15 14:28:25]

https://codereview.adblockplus.org/29692555/diff/29692556/popup.html
File popup.html (left):

https://codereview.adblockplus.org/29692555/diff/29692556/popup.html#oldcode86
popup.html:86: <span class="i18n_stats_share_title"></span>
On 2018/02/15 09:43:10, a.giammarchi wrote:
> On 2018/02/14 17:19:46, saroyanm wrote:
> > We still have this String in the "locales".
> > Removing it only from the en_US should be sufficient, translation script
> should
> > take care of removing it for the other locales.
> 
> This is a different repository. I can do that only as follow up / another
> ticket.

We didn't yet migrated popup.html from the ABPchrome repository that's  true,
but we do currently keep the strings of non migrated pages in the ABPchrome
still, in messages.json, hope we will migrate all soon to make things more
intuitive:
https://hg.adblockplus.org/adblockpluschrome/file/tip/_locales/en_US/messages...

https://codereview.adblockplus.org/29692555/diff/29692556/skin/popup.css
File skin/popup.css (left):

https://codereview.adblockplus.org/29692555/diff/29692556/skin/popup.css#oldc...
skin/popup.css:296: background-image: url(popup.png);
On 2018/02/15 09:43:10, a.giammarchi wrote:
> On 2018/02/14 17:19:47, saroyanm wrote:
> > Detail/Nit: We still have the images in the popup.png file.
> > 
> > While it will require adjustment of the icons if we crop the image, I will
> > suggest to only remove the icons from the file keeping a transparent place.
> 
> Apologies I don't understand this comment or what should I do exactly.
> Please expand a little bit more, thanks.

I was suggesting to remove unused icons (FB, Twitter, G+) from the sprite ->
https://github.com/adblockplus/adblockpluschrome/blob/master/skin/popup.png 

But same time do not to change the position of other icons(checkboxes, close,
statistics and etc.).

[a.giammarchi, 2018-02-15 16:02:43]

https://codereview.adblockplus.org/29692555/diff/29692556/popup.html
File popup.html (left):

https://codereview.adblockplus.org/29692555/diff/29692556/popup.html#oldcode86
popup.html:86: <span class="i18n_stats_share_title"></span>
On 2018/02/15 14:28:24, saroyanm wrote:
> On 2018/02/15 09:43:10, a.giammarchi wrote:
> > On 2018/02/14 17:19:46, saroyanm wrote:
> > > We still have this String in the "locales".
> > > Removing it only from the en_US should be sufficient, translation script
> > should
> > > take care of removing it for the other locales.
> > 
> > This is a different repository. I can do that only as follow up / another
> > ticket.
> 
> We didn't yet migrated popup.html from the ABPchrome repository that's  true,
> but we do currently keep the strings of non migrated pages in the ABPchrome
> still, in messages.json, hope we will migrate all soon to make things more
> intuitive:
>
https://hg.adblockplus.org/adblockpluschrome/file/tip/_locales/en_US/messages...
> 

Done.

https://codereview.adblockplus.org/29692555/diff/29692556/skin/popup.css
File skin/popup.css (left):

https://codereview.adblockplus.org/29692555/diff/29692556/skin/popup.css#oldc...
skin/popup.css:296: background-image: url(popup.png);
On 2018/02/15 14:28:24, saroyanm wrote:
> On 2018/02/15 09:43:10, a.giammarchi wrote:
> > On 2018/02/14 17:19:47, saroyanm wrote:
> > > Detail/Nit: We still have the images in the popup.png file.
> > > 
> > > While it will require adjustment of the icons if we crop the image, I will
> > > suggest to only remove the icons from the file keeping a transparent
place.
> > 
> > Apologies I don't understand this comment or what should I do exactly.
> > Please expand a little bit more, thanks.
> 
> I was suggesting to remove unused icons (FB, Twitter, G+) from the sprite ->
> https://github.com/adblockplus/adblockpluschrome/blob/master/skin/popup.png 
> 
> But same time do not to change the position of other icons(checkboxes, close,
> statistics and etc.).

Done.

[a.giammarchi, 2018-02-15 16:04:51]

FYI for some reason HG decided these two where different but my current exported
diff is actually a single one.

[saroyanm, 2018-02-15 16:31:25]

On 2018/02/15 16:04:51, a.giammarchi wrote:
> FYI for some reason HG decided these two where different but my current
exported
> diff is actually a single one.

Maybe you have committed the old changes ?

Anyway considering that the html and js files are unchanged this is LGTM

[kzar, 2018-02-16 18:19:06]

Can't we also remove the strings "stats_over", "stats_share_download" and
"stats_share_message"
 now?

Also Thomas do you want to review this change before it lands, or are you happy
with a review from me and Manvel?

[kzar, 2018-02-16 18:26:32]

Oh, I reviewed this using the patch that Andrea emailed over to me. But it looks
like there's a problem on this review. 

Andrea did you upload the entire change for the second patch set? With Rietveld
you have to upload the entire change every time, Rietveld then takes care of
figuring out the inter-diffs. I ask since I can only see the popup.js changes in
the first patch set.

[a.giammarchi, 2018-02-19 08:37:55]

On 2018/02/16 18:26:32, kzar wrote:
> Oh, I reviewed this using the patch that Andrea emailed over to me. But it
looks
> like there's a problem on this review. 
> 
> Andrea did you upload the entire change for the second patch set? With
Rietveld
> you have to upload the entire change every time, Rietveld then takes care of
> figuring out the inter-diffs. I ask since I can only see the popup.js changes
in
> the first patch set.

I think I've messed up badly while trying to push so I might re-create the patch
from the scratch and send it out double checked and clean after making last
changes.

This unless I manage to find a way to squash all changes in here, which so far I
haven't managed at all.

Sorry for the inconvenience

[a.giammarchi, 2018-02-19 09:50:24]

Patch Set 2 - Cleaned up previous patch 2 and addressed Manvel and Dave's
feedback.

[kzar, 2018-02-19 09:54:19]

LGTM

[saroyanm, 2018-02-20 11:35:22]

LGTM

[kzar, 2018-02-20 12:01:22]

https://hg.adblockplus.org/adblockpluschrome/rev/b93c23bea12b