#6321 closed change (duplicate)

Move popup.html and related files to adblockplusui

Reported by: greiner Assignee: agiammarchi
Priority: P3 Milestone:
Module: User-Interface Keywords:
Cc: kzar, sebastian Blocked By:
Blocking: #6322 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29801613/

Description (last modified by greiner)

Background

The icon popup belongs to the User Interface module but the code for it is still located in adblockpluschrome. Therefore we want to move all its code to adblockplusui to be able to offer a more streamlined interface with the Web Extension team.

However, it turns out that this also affects notification.js and stats.js which are not explicitly listed on adblockplus.org/modules but due to their strong ties to the UI, it'd make sense to move them as well.
Unfortunately, they're not using the messaging API yet which makes this separation a bit more tedious than hoped but this can be worked on separately.

Another question would be to which module the logo icons belong since we use both adblockpluschrome/icons/detailed/abp-64.png as well as adblockplusui/skin/abp-128.png in the popup which is not ideal.

What to change

  • Move the following files from adblockpluschrome to adblockplusui:
    • icons/detailed/abp-64.png
    • skin/popup.css
    • skin/popup.png
    • popup.html
    • popup.js
  • Create or extend the mock implementation of the following files/modules accordingly in adblockplusui:
    • ext/popup.js
    • filterClasses
    • filterNotifier
    • filterStorage
    • notification
    • notificationHelper
    • prefs
    • stats
    • url
    • utils
    • whitelisting

Change History (9)

comment:1 Changed 22 months ago by sebastian

Is this effort worth it? You currently redesign the popup anyway, right? So how about doing it the same way as we did with the options page, i.e. developing the new popup in adblokcplusui and then removing the legacy implemantion from adblockpluschrome once we integrate the new popup?

comment:2 Changed 22 months ago by greiner

  • Blocking 6322 added

That's another good approach. There are barely any details about how such a redesign would look like and should be done.

But fair enough. Let's assume we will write it from scratch. In that case we'd only need to worry about where the icons should belong to.

comment:3 Changed 18 months ago by greiner

See also #6738.

comment:4 Changed 18 months ago by greiner

  • Description modified (diff)

Updated ticket description to reflect that notification.js and stat.js have been merged into popup.js (see cd17039102e2).

comment:5 Changed 18 months ago by greiner

  • Owner set to agiammarchi
  • Priority changed from Unknown to P3
  • Ready set

comment:6 Changed 18 months ago by greiner

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:7 Changed 15 months ago by greiner

@agiammarchi Since this change will be part of #6892, mind if we close this one as duplicate and tackle all the necessary changes for the dependency update there?

For reference, the corresponding changes to our adblockpluschrome fork have landed in this commit.

comment:8 Changed 15 months ago by agiammarchi

@greiner works for me

comment:9 Changed 15 months ago by greiner

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

Closing this ticket as duplicate of #6892.

Note: See TracTickets for help on using tickets.