Opened 6 months ago

Closed 3 days ago

Last modified 3 days ago

#5613 closed change (fixed)

Add an option to hide the "Share this number" and the social media buttons from the ABP toolbar button menu

Reported by: darkblue Assignee: agiammarchi
Priority: P3 Milestone: Adblock-Plus-for-Chrome-Opera-Firefox-next
Module: User-Interface Keywords: goodfirstbug
Cc: greiner, martin, jeen, kzar, saroyanm Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29692555

Description (last modified by greiner)

Environment

Using Chrome 60 with ABP stable 1.13.3
and Firefox Nightly 57 with ABP dev 2.99.0.1832
in windows 10.

Background

Currently in ABP stable for Chrome as well as in ABP dev (WebExtension) for Firefox Nightly
when you click the ABP toolbar button and so the menu appears,
there is a "Share this number" text and social media buttons,
below the blocked ads counters "x on this page, y in total".

For reference, ABP stable for Firefox never had this text and buttons in its toolbar button menu.
I understand their purpose, but I'd like to have to option to hide them.

Here is a screenshot from ABP stable for chrome and a mockup I made with my suggestion:
https://i.imgur.com/qeYQgU5.jpghttps://i.imgur.com/oQpTWJE.jpg

See also https://adblockplus.org/forum/viewtopic.php?f=4&t=54027

What to change

Remove #share element from icon popup and any code it depends on.

Change History (22)

comment:1 Changed 6 months ago by greiner

  • Cc greiner added
  • Component changed from Unknown to User-Interface
  • Description modified (diff)

I'd imagine that we could introduce such a preference if we accomplish to nicely integrate this into the UI. Another option would be to get rid of the share section alltogether in case it turns out that it's not providing enough value but that might be difficult to determine.

Anyway, I'll check back with our Product team to see what they think.

comment:2 Changed 3 months ago by Anke

I also wish, ABP's share buttons were optional.

Being able to block social buttons on websites while at the same time providing share buttons with tracking potential within the extension that cannot be deactivated seems rather non-credible.

comment:3 Changed 3 months ago by greiner

  • Cc martin jeen added

comment:4 Changed 3 months ago by kzar

  • Cc kzar added

FWIW these buttons seem pointless to me as well, why would you ever share a message like "I blocked 23 adverts today" with your friends?

comment:5 Changed 3 months ago by jeen

Martin has been working on a new UI update for the flyout menu which will remove this feature

comment:6 Changed 3 months ago by saroyanm

  • Cc saroyanm added

comment:7 Changed 5 weeks ago by greiner

@jeen Since the new UI will still be a few months away, what about already removing the social media buttons? That way we could also see what impact removing it might have in which case we could take that into consideration before releasing the new UI.

comment:8 Changed 5 weeks ago by jeen

@greiner Good idea - the new design already has it removed anyway, and I would be in favour of removing it from the existing design too.

comment:9 Changed 5 weeks ago by greiner

  • Description modified (diff)
  • Keywords goodfirstbug added
  • Priority changed from Unknown to P3
  • Ready set

comment:10 Changed 2 weeks ago by agiammarchi

  • Owner set to agiammarchi

comment:11 Changed 2 weeks ago by agiammarchi

  • Review URL(s) modified (diff)

comment:12 Changed 2 weeks ago by greiner

  • Status changed from new to reviewing

comment:13 follow-up: Changed 3 days ago by abpbot

A commit referencing this issue has landed:
Issue 5613 - Removal of social media: Share this number

comment:14 Changed 3 days ago by agiammarchi

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:15 in reply to: ↑ 13 Changed 3 days ago by saroyanm

Replying to abpbot:

A commit referencing this issue has landed:
Issue 5613 - Removal of social media: Share this number

Note: The updated image is missing from the commit. I assume it's because of the Rietvield, exported, patch, we do have a script that helps including binary files as well, see -> https://hg.adblockplus.org/codingtools/file/tip/patchconv/README.md#l9

comment:16 follow-up: Changed 3 days ago by agiammarchi

The patch already landed in master so I guess we eventually need to push only the updated image? I can send the image to Dave since I don't have push rights to their repo.

FWIW I think our workflow is not ideal, I hope once in GitLab things will be simplified in a less error prone way.

comment:17 in reply to: ↑ 16 Changed 3 days ago by saroyanm

Replying to agiammarchi:

The patch already landed in master so I guess we eventually need to push only the updated image? I can send the image to Dave since I don't have push rights to their repo.

Yes, please just be sure that the remaining icons looks fine(are not displaced) and separate push should work.

FWIW I think our workflow is not ideal, I hope once in GitLab things will be simplified in a less error prone way.

Agree.

comment:18 Changed 3 days ago by abpbot

A commit referencing this issue has landed:
Issue 5613 - Removed social media icons

comment:19 Changed 3 days ago by kzar

Damn, well spotted. Both diffs were missing the image changes but I didn't notice. Not to worry anyway, I've taken that image change from Rietveld and pushed it for you Andrea.

comment:20 Changed 3 days ago by agiammarchi

Thank you. The hg export basically forgets about binaries, simply pointing at one revision hash and a file name. I should've asked if that was OK or not.

just be sure that the remaining icons looks fine

sure, only social logos are gone leaving the alpha channel underneath and nothing else ;-)

comment:21 Changed 3 days ago by kzar

So we got to the bottom of it, Andrea had used an OK command to produce the patch (something like hg export > the-name.patch) but since he didn't have the recommended Mercurial configuration the extended Git diff format wasn't used (the --git switch would have worked too) and so the binary files were skipped.

Edit: I should say I also missed that the binary files were skipped when I checked the patches, I should have spotted that sorry.

Last edited 3 days ago by kzar (previous) (diff)

comment:22 Changed 3 days ago by agiammarchi

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
Note: See TracTickets for help on using tickets.