-
Notifications
You must be signed in to change notification settings - Fork 398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add events to disco pane #543
Add events to disco pane #543
Conversation
return { | ||
handleGlobalEvent(e) { | ||
const { id, type, needsRestart } = e; | ||
const payload = { guid: id, needsRestart }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice if this mostly moved into addonManager
. So this could be:
componentDidMount() {
addonManager.addChangeListener(handleGlobalEvent);
}
return {
handleGlobalEvent(type, payload) {
dispatch({type, payload});
}
};
|
||
Object.keys(eventMap).forEach((event) => { | ||
const action = eventMap[event]; | ||
it(`dispatches ${action}`, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dispatch might not be the best word to use for this. I think the callback actually calls dispatch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a bit of a leftover from refactoring, I'll update it.
r+wc |
0c06075
to
32918b2
Compare
|
I'll file a bug separately for making the stubAddonManager thing more distinct. |
function handleChangeEvent(e) { | ||
const { id, type, needsRestart } = e; | ||
log.info('Event received', {type, id, needsRestart}); | ||
if (globalEventStatusMap.hasOwnProperty(type)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this should be a Map
if it has Map
in the name. Unfortunately it looks like Map
needs to be initialized with an array of key/value pairs but that's probably still nicer than using a plain object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't feel too strongly it must be a map, since it's just a lookup.
Fixes mozilla/addons#9663
Fixes mozilla/addons#9688