-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Bug 856888 - Support links was added in calendar's notes #33545
base: master
Are you sure you want to change the base?
Bug 856888 - Support links was added in calendar's notes #33545
Conversation
@@ -0,0 +1,32 @@ | |||
/*global ActivityPicker */ |
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.
This should use amd like our other modules. ie
define(function(require, exports, module) {
'use strict';
module.exports = function lah_onClick(event) {
// ...
};
});
**What was made: ** * Move modules that provides convertion telephone numbers, emails and links to links in messages to shared component from sms. * Add this functionality in calendar's notes.
4701c2c
to
3609ecc
Compare
/* global ActivityPicker */ | ||
|
||
define(function(require, exports, module) { | ||
'use strict'; |
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.
nit: Move code inside module 2 spaces left
I think I'd prefer that you recreate a simpler ActivityPicker inside the Calendar app. As you see, the full ActivityPicker you moved is used for other things than only the links handler (see in apps/sms/views/conversation/js/conversation.js for example). You could even recreate it directly in link_action_handler.js. Nowadays we can also use MozActivity like a promise, so we don't need to use onsuccess/onerror like we do in activity_picker. We can merely return |
What was made: