Opened 4 years ago

Closed 4 years ago

#1965 closed change (fixed)

Refactor icon animation code

Reported by: sebastian Assignee: sebastian
Priority: P4 Milestone: Adblock-Plus-1.8.11-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: Blocked By:
Blocking: Platform: Unknown
Ready: yes Confidential: no
Tester: Verified working:
Review URL(s):

http://codereview.adblockplus.org/5196306347720704

Description (last modified by sebastian)

Background

There are currently some issues with the approach of the code animating the icon to indicate notifications.

What to change

The logic to generate the icon's filename is currently spread into two places, where the code handling the icon animation injects the -notification-<type>-<frame> suffix with a regex. This is a rather ugly hack. Moreover, it breaks when generating the whitelisted icon overlayed with a notification icon at full opacity. A better approach would be to combine the logic to generate the icon's filename into a single function, using simple string concatenation.

Also the icon animation is currently implemented with a global object. So while on it, we should make it a proper module, to improve encapsulation and to makes things more consistent.

Hints for testers

In order to test the icon animation (after this changes has been merged), go to chrome://extensions, activate "Developer Mode", inspect the extension's background page, and run startIconAnimation("critical").

Change History (4)

comment:1 Changed 4 years ago by sebastian

  • Summary changed from Rafactor icon animation code to Refactor icon animation code

comment:2 Changed 4 years ago by sebastian

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

comment:3 Changed 4 years ago by sebastian

  • Description modified (diff)

comment:4 Changed 4 years ago by sebastian

  • Milestone set to Adblock-Plus-1.8.11-for-Chrome-Opera-Safari
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.