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

Include useKrispNoiseFilter in generated docs, + related improvements #1055

Merged
merged 24 commits into from
Dec 21, 2024

Conversation

bcherry
Copy link
Contributor

@bcherry bcherry commented Dec 18, 2024

The main issue here is the krisp hook was excluded from generated docs because it's not actually in the react package index.d.ts. I decided the most straightforward thing to do is include it in generated docs anyways alongside everything else. I improved its docs a bit and upgraded it from alpha to beta.

While working on this I found and fixed the following additional issues:

  • Added a custom @package tag for tsdoc so we can override the import path for the krisp hook
  • Turn the @beta tag into a callout instead of a paragraph (and some text edits)
  • Add a callout for the @alpha tag just like the @beta tag has
  • We automatically unrolled parameters with type names ending in "Props" but we also sometimes use "Options" instead, including for useKrispNoiseFilter, so I added that.
  • Unrolled parameters are confusing when there are other parameters in the list as well so I also added a prefix to them indicating where they should be included (e.g. options.abc). this still has room for improvement
  • The @return directive is currently ignored entirely in favor of a generated signature. To improve docs, I've added the return description, if supplied, alongside the generated code.
  • Multi-example docs were kinda unnatural with totally separate sections labeled "Usage Example 1", "Usage Example 2", etc. I changed it to be one section called Usage, with sub-headers for individual examples. They now use their first line as the title, falling back to numbered examples. I added extra examples to useIsMuted with this change.
image image

@bcherry bcherry requested review from Ocupe and lukasIO December 18, 2024 23:05
Copy link

changeset-bot bot commented Dec 18, 2024

🦋 Changeset detected

Latest commit: ae92d3a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@livekit/components-core Patch
@livekit/components-react Patch
@livekit/api-documenter Patch
@livekit/component-example-next Patch
@livekit/components-js-docs Patch
@livekit/component-docs-storybook Patch
@livekit/components-docs-gen Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Dec 18, 2024

size-limit report 📦

Path Size
LiveKitRoom only 6.05 KB (0%)
LiveKitRoom with VideoConference 29.82 KB (0%)
All exports 36.51 KB (0%)

@bcherry bcherry changed the title Include useKrispNoiseFilter in generated docs, add alpha callout, improve beta callout Include useKrispNoiseFilter in generated docs, + related improvements Dec 18, 2024
Copy link
Contributor

@lukasIO lukasIO left a comment

Choose a reason for hiding this comment

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

looks good to me, but will defer to @Ocupe

*/
trackRef?: TrackReferenceOrPlaceholder;
/**
* @internal
Copy link
Contributor

Choose a reason for hiding this comment

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

why would they be internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the easiest way to just leave it out of the docs. this comes from the closed-source underlying krisp SDK, so there's no good way to document this parameter. the actual values available to tweak here also seem unlikely to be useful, it's just debugging flags basically? I don't feel strongly about it I just thought it's easier to exclude it from the docs.

@@ -209,6 +208,9 @@ export class MarkdownDocumenter {
if (apiItem.releaseTag === ReleaseTag.Beta) {
this._writeBetaWarning(output);
}
if (apiItem.releaseTag === ReleaseTag.Alpha) {
this._writeAlphaWarning(output);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

Copy link
Contributor

@Ocupe Ocupe left a comment

Choose a reason for hiding this comment

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

Looking good 👍

@bcherry bcherry merged commit 146272c into main Dec 21, 2024
3 checks passed
@bcherry bcherry deleted the bcherry/krisp-hook-doc branch December 21, 2024 03:56
@github-actions github-actions bot mentioned this pull request Dec 14, 2024
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.

3 participants