-
Notifications
You must be signed in to change notification settings - Fork 48
Conversation
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.
Thanks for this addition @Steveb-p!
The PR looks good to me, could you just rebase it on top of the latest master? We have recently switched to TypeScript, have replaced ESLint with TSLint and have also introduced Prettier (#21). Your changes should fit well on top of that, the name of the file just changes from .js
to .ts
.
You can run yarn format
before making the commit to make sure CI does not fail when checking code style.
Once your PR is merged, @denisnikulin or I will create a new PR to release 0.8, which will include your change. A reminder to myself: don't forget about the changelog.
I've noticed that PR, but it was already too late and PR was submitted :) I'll gladly do the rebase. By the way, is there any reason why multiple audio elements are forbidden? Exposing audio element would potentially allow usage of audio filters maybe as well, so I was thinking of exposing ref of created element for this reason. |
We can discuss passing the ref down in a separate issue or a PR if you want – I'd be keen to see a usage example. Now that you give the element a fixed ID, won't that be enough for configuring filters etc.? As of a single |
In my app I've changed the SipProvider to use new context API and changed the render function as follows: render() {
return (
<React.Fragment>
<audio ref={el => this.audioRef = el} />
<SipContext.Provider value={{
audioElement: this.audioRef,
registerSip: this.registerSip,
unregisterSip: this.unregisterSip,
answerCall: this.answerCall,
startCall: this.startCall,
stopCall: this.stopCall,
}}>
{ this.props.children }
</SipContext.Provider>
</React.Fragment>
);
} This way there is no introduction of new id to window and audio element is available in and only in relevant context. I'm still wondering if re-rendering of audio element will be causing issues, since I haven't yet checked. Otherwise, I would just pass the id as prop to allow using whatever id one wants, and to allow multiple SipProviders. As for use case, the one I'm going for is putting calls on hold and making new ones while the original is still pending. It might be invalid approach, but I'm relatively new to SIP protocol and related RTC. EDIT: Properties in sample are not passed down, because I've made them part of redux state, but you get the idea. |
Can UPD: let's keep the context-related conversation in #19 |
It's just a sample. Not really relevant to render() {
return (
<React.Fragment>
<audio ref={el => this.audioRef = el} />
{ this.props.children }
</React.Fragment>
);
} For it to work it would mean checking against React.createContext function presence and switching between old and new implementation, I guess. |
@Steveb-p we'll be releasing a new version today – do you have an opportunity to rebase the existing changes? We can think of further improvements later I guess. |
@kachkaev rebased and changes reapplied to index.ts |
Thanks for doing this so quickly @Steveb-p! Could you please run |
This change adds ID attribute to audio element when attaching to document, checks against existence of one such element during component mounting and properly removes audio element when SipProvider is unmounted.
@kachkaev done. I figure other reformatted files are supposed to be left alone, so I did ;) (some .md and package.json) |
@Steveb-p awesome, many thanks! LGTM 🎉 |
0.8.0 is out 😉 |
This change adds ID attribute to audio element when attaching to document, checks against existence of one such element during component mounting and properly removes audio element when SipProvider is unmounted.