Opened on 01/26/2018 at 02:23:02 PM
Closed on 09/07/2018 at 05:22:09 PM
#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): | |||
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
Attachments (0)
Change History (9)
comment:1 Changed on 01/26/2018 at 02:26:10 PM by sebastian
comment:2 Changed on 01/26/2018 at 02:32:40 PM 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:4 Changed on 06/08/2018 at 01:30:58 PM 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 on 06/08/2018 at 01:34:22 PM by greiner
- Owner set to agiammarchi
- Priority changed from Unknown to P3
- Ready set
comment:6 Changed on 06/08/2018 at 01:34:32 PM by greiner
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:7 Changed on 09/07/2018 at 02:49:40 PM 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 on 09/07/2018 at 03:31:30 PM by agiammarchi
@greiner works for me
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?