Skip to content
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 Transceiver guide #56

Merged
merged 9 commits into from
Jan 16, 2024
Merged

Add Transceiver guide #56

merged 9 commits into from
Jan 16, 2024

Conversation

mickel8
Copy link
Member

@mickel8 mickel8 commented Jan 10, 2024

No description provided.

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

Merging #56 (b985785) into master (8bf58fa) will not change coverage.
Report is 1 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #56   +/-   ##
=======================================
  Coverage   89.02%   89.02%           
=======================================
  Files          25       25           
  Lines        1057     1057           
=======================================
  Hits          941      941           
  Misses        116      116           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bf58fa...b985785. Read the comment docs.

@mickel8 mickel8 mentioned this pull request Jan 10, 2024
54 tasks
@mickel8 mickel8 force-pushed the tr-guide branch 4 times, most recently from 4c8ac5e to 3d06d29 Compare January 11, 2024 19:26
@mickel8 mickel8 marked this pull request as ready for review January 11, 2024 19:28
@mickel8 mickel8 requested a review from LVala January 11, 2024 19:28
Copy link
Member

@LVala LVala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some general remarks:

  • use Grammarly or paste the text into ChatGPT and tell it to fix the grammar and punctuation,
  • like I mentioned somewhere, I feel like the guide is missing a glossary, as a lot of the terminology might be obvious for us but not necessarily for someone who just used the JS API and didn't really look into SDP and stuff. I would also move the explanations of transceiver properties (direction etc.) there,
  • the guide goes quite in-depth from the very beginning, I would love to see some introductory example (maybe showing differences between just using addTrack and using transceivers directly)
  • maybe it would be nice to crate a section where all of the API functions used later on are explained (focusing on intuition),
  • link to some introductory guide to WebRTC API in the beginning of the guide?
  • what do you think of adding a nice diagram showing relations between transceivers/senders/transports etc. with the new ex_doc diagram feature?

That's it for now, but if anything comes to my mind regarding the guide, I'm going to share it.

guides/transceiver_guide.md Outdated Show resolved Hide resolved
guides/transceiver_guide.md Outdated Show resolved Hide resolved
guides/transceiver_guide.md Outdated Show resolved Hide resolved
guides/transceiver_guide.md Outdated Show resolved Hide resolved
guides/transceiver_guide.md Outdated Show resolved Hide resolved
guides/transceiver_guide.md Outdated Show resolved Hide resolved
guides/transceiver_guide.md Outdated Show resolved Hide resolved
guides/transceiver_guide.md Outdated Show resolved Hide resolved
guides/transceiver_guide.md Outdated Show resolved Hide resolved
guides/transceiver_guide.md Outdated Show resolved Hide resolved
@mickel8
Copy link
Member Author

mickel8 commented Jan 12, 2024

Some general remarks:

use Grammarly or paste the text into ChatGPT and tell it to fix the grammar and punctuation,

done

like I mentioned somewhere, I feel like the guide is missing a glossary, as a lot of the terminology might be obvious for us but not necessarily for someone who just used the JS API and didn't really look into SDP and stuff. I would also move the explanations of transceiver properties (direction etc.) there,

the guide goes quite in-depth from the very beginning, I would love to see some introductory example (maybe showing differences between just using addTrack and using transceivers directly)

maybe it would be nice to crate a section where all of the API functions used later on are explained (focusing on intuition),
what do you think of adding a nice diagram showing relations between transceivers/senders/transports etc. with the new ex_doc diagram feature?

Great point! However, this is quite a lot of work and the idea was to have more advanced guide for people who more or less know what webrtc is and are looking for deep understanding of transceivers and what they can do with them.

I think that all of the things you listed i.e. glossary, more basic examples, diagrams, API explanation is for another section(s) and for another task.

For now I would favor renaming the guide to Mastering Transceivers or something like this and stating at the beginning what's the purpose of this guide.

link to some introductory guide to WebRTC API in the beginning of the guide?

👍

@LVala
Copy link
Member

LVala commented Jan 12, 2024

Great point! However, this is quite a lot of work and the idea was to have more advanced guide for people who more or less know what webrtc is and are looking for deep understanding of transceivers and what they can do with them.

I think that all of the things you listed i.e. glossary, more basic examples, diagrams, API explanation is for another section(s) and for another task.

For now I would favor renaming the guide to Mastering Transceivers or something like this and stating at the beginning what's the purpose of this guide.

Sure

@mickel8 mickel8 requested a review from LVala January 12, 2024 13:20
Copy link
Member

@LVala LVala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I said earlier, maybe it would be nice to get Jellyfish guys to read through this to get and outsider's perspective, but otherwise -- sgtm.

guides/mastering_transceivers.md Outdated Show resolved Hide resolved
guides/mastering_transceivers.md Outdated Show resolved Hide resolved
guides/mastering_transceivers.md Outdated Show resolved Hide resolved
guides/mastering_transceivers.md Show resolved Hide resolved
@mickel8 mickel8 requested a review from LVala January 15, 2024 17:34
Copy link
Member

@LVala LVala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly, I suppose it shouldn't look like this...
image

Also, I realised that, in the JS examples, you don't use const or let keywords, which is fine for console I guess, but you will need to add it when copying the examples to standalone script (at least I believe so, I'm no JS expert).

@mickel8 mickel8 merged commit 528c056 into master Jan 16, 2024
4 checks passed
@mickel8 mickel8 deleted the tr-guide branch January 16, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants