Opened 15 months ago

Closed 10 months ago

Last modified 9 months 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-3.0.3-for-Chrome-Opera-Firefox
Module: User-Interface Keywords: goodfirstbug
Cc: greiner, martin, jeen, kzar, saroyanm Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
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 (26)

comment:1 Changed 15 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 13 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 13 months ago by greiner

  • Cc martin jeen added

comment:4 Changed 13 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 13 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 12 months ago by saroyanm

  • Cc saroyanm added

comment:7 Changed 11 months 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 11 months 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 11 months ago by greiner

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

comment:10 Changed 10 months ago by agiammarchi

  • Owner set to agiammarchi

comment:11 Changed 10 months ago by agiammarchi

  • Review URL(s) modified (diff)

comment:12 Changed 10 months ago by greiner

  • Status changed from new to reviewing

comment:13 follow-up: Changed 10 months ago by abpbot

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

comment:14 Changed 10 months ago by agiammarchi

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

comment:15 in reply to: ↑ 13 Changed 10 months 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 10 months 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 10 months 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 10 months ago by abpbot

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

comment:19 Changed 10 months 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 10 months 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 10 months 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 10 months ago by kzar (previous) (diff)

comment:22 Changed 10 months ago by agiammarchi

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next

comment:23 Changed 9 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Share icons have been removed and their removal has not caused any issues.

Firefox 53 / Firefox 58 / Windows 7
Chrome 49 / 65 / Windows 7
Opera 36 / 51 / Windows 7

comment:24 follow-up: Changed 9 months ago by agiammarchi

edit: discussed already, irrelevant question

Thank you Ross. Just one question: AFAIK Firefox ESR (Extended Support Release) 'till May 8 is based on Firefox 52. Is such version of Firefox supported too or something we can ignore in ABP UI?

This is because I've seen your FF version range from 53, not 52. I am not expecting any surprise/issue in FF 52 but for us it's kinda crucial to know exact versioning of the browsers we want to support.

Chrome and Opera match our expectations.

Regards

Last edited 9 months ago by agiammarchi (previous) (diff)

comment:25 in reply to: ↑ 24 ; follow-up: Changed 9 months ago by kzar

Replying to agiammarchi:

...Firefox 52. Is such version of Firefox supported too or something we can ignore in ABP UI?

The best place to check the supported browser versions is the metadata files in adblockpluschrome. For example adblockpluschrome/devenv.gecko shows that 51.0 is the earliest Firefox version we support.

comment:26 in reply to: ↑ 25 Changed 9 months ago by agiammarchi

Replying to kzar:

The best place to check the supported browser versions is the metadata files in adblockpluschrome. For example adblockpluschrome/devenv.gecko shows that 51.0 is the earliest Firefox version we support.

Thanks for the link, that is what I was referring too about FF version.
We've clarified few things in IRC already so my question has been already answered there, I should've updated in here too.

Note: See TracTickets for help on using tickets.