[VBLOCKS 3456] feat: hold resume #14
Conversation
kpchoy
force-pushed
the
VBLOCKS-3456-hold-resume
branch
from
a5f37ed to
8e7c595
Compare
| twilioVoiceDialer.addEventListener('tokenWillExpire', async (e) => { | ||
| const device = e.detail.device; | ||
|
|
||
| const updateTokenResponse = await fetch(`/twilio-voice-dialer/token`); |
There was a problem hiding this comment.
| const updateTokenResponse = await fetch(`/twilio-voice-dialer/token`); | |
| const updateTokenResponse = await fetch('/twilio-voice-dialer/token'); |
| #call; | ||
| #callSid; | ||
| #conferenceSid; |
There was a problem hiding this comment.
It seems like there are two different "unset" values for these. Later we set them to null, but here they are undefined. We should settle on undefined IMO.
There was a problem hiding this comment.
Makes sense. Any reason for undefined over null?
There was a problem hiding this comment.
Purely because leaving it "blank" sets the value to undefined. Consider the following:
const foo = null;
const bar = undefined;
typeof foo === 'object'; // confusing
typeof bar === 'undefined'; // less confusing
if (foo === null) { ... } // works
if (bar === undefined) { ... } // does not work, very confusing
So both of them have pretty confusing cons, and the benefit is that we just don't have to set things to null by hand things will be implicit undefined.
| this.#setHold(false); | ||
| } | ||
|
|
||
| #messageReceivedHandler(message) { |
There was a problem hiding this comment.
Nitpick: since all the other handlers are handleX, let's call this one handleCallMessageReceived?
| @@ -7,7 +7,7 @@ const { accountSid, apiKeySid, apiKeySecret } = config; | |||
| const client = Twilio(apiKeySid, apiKeySecret, { accountSid }); | |||
|
|
|||
| router.post('/conferences/:conferenceSid/participants/:callSid', async (req, res) => { | |||
| const { hold } = req.body; | |||
| const { hold } = JSON.parse(req.body); | |||
There was a problem hiding this comment.
I think if you set the headers to application/json in the fetch, you don't need to do JSON.parse here.
| @@ -9,7 +9,9 @@ | |||
| <body> | |||
| <h1>Twilio Voice Dialer</h1> | |||
|
|
|||
| <twilio-voice-dialer recipient register></twilio-voice-dialer> | |||
| <twilio-voice-dialer recipient register> | |||
| <twilio-voice-basic-call-control></twilio-voice-basic-call-control> | |||
There was a problem hiding this comment.
We don't want this reference to have the hold and resume functionality right? This is the voice dialer without any conference/call control stuff?
There was a problem hiding this comment.
good catch. must have missed this in my rebase
There was a problem hiding this comment.
@kpchoy I left a couple of comments. I understand this has already been merged. Please add a new ticket to address the comments later 🙇
fyi @mhuynh5757
| <h1>Twilio Voice Dialer</h1> | ||
|
|
||
| <twilio-voice-dialer recipient register> | ||
| <twilio-voice-basic-call-control></twilio-voice-basic-call-control> |
There was a problem hiding this comment.
Will a self closing tag work? <twilio-voice-basic-call-control/>
There was a problem hiding this comment.
Sadly Custom elements cannot be self closing
WICG/webcomponents#624
| this.#render(); | ||
| this.#showButtons(); | ||
|
|
||
| const twilioVoiceDialer = document.querySelector('twilio-voice-dialer'); |
There was a problem hiding this comment.
This will query the whole document. We should only query the child nodes of this component. Can you try this.shadowRoot.querySelector?
There was a problem hiding this comment.
The issue here is that this.shadowRoot is scoped to this component. So it has no reference of parent element, where the event is dispatched. Hence why we need to query the whole document for the parent element.
There was a problem hiding this comment.
I was confused for a bit. Yeah this is the child element, and the dialer is the parent. In this case, this is still a problem because what if you have multiple dialers in the document?
There was a problem hiding this comment.
Valid point. Ok, created this follow up ticket to "pass down UUID from Dialer to child components"
https://twilio-engineering.atlassian.net/browse/VBLOCKS-3743
There was a problem hiding this comment.
There was a problem hiding this comment.
@charliesantos 1 line fix: #20
There was a problem hiding this comment.
Great! Approved. I remember you created a jira ticket to pass the id. Can we close that now since it's not applicable anymore?
VBLOCKS-3540
VBLOCKS-3546
Contributing to Twilio