search

mozilla / addon-recommendation-shield-study

Stand-alone verison of Add-on Recommendation for Shield Study
Mozilla Public License 2.0
3 stars 7 forks source link

Keep track of panel state, don't always call "showPanel" #68

Closed casebenton closed 8 years ago

casebenton commented 8 years ago

@Osmose r?

Osmose commented 8 years ago

The issue number in the commit message is wrong.

Osmose commented 8 years ago

Assuming you meant #66, this is an okay solution, but there's something you can use that is better; panel events.

The panel docs mention events that you can subscribe to using the on method. Instead of adding more state to track whether the panel has been shown or not, you should instead remove the tracking of panel shows from the button click handler, and instead register a listener for when the panel is shown and log to telemetry there. Then you have less state to track and can be confident that panel shows are logged in all situations.

What do you think?

casebenton commented 8 years ago

I agree that this is a better solution. It felt risky to have to keep track of state that the sdk/panel was certainly also storing. I'm doing a variation of this already for hide events. In the panel constructor method, one of the properties of the object passed to the constructor specifies my method to call on panel hide events. I'll use this technique for the show event too for symmetry.

casebenton commented 8 years ago

@Osmose I'm using the sdk/panel's event listener for panel open events now. r?

Osmose commented 8 years ago

Much better. This looks great, just rebase the two commits into a single commit please, and I'm r+ on this. Nice work!