-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
docs: Add missing mask option to documentation #6625
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
base: main
Are you sure you want to change the base?
Conversation
…rify mask behavior
📝 WalkthroughWalkthroughThis PR updates documentation to properly expose the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit d85bf52
☁️ Nx Cloud last updated this comment at |
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/router/framework/react/guide/navigation.md (1)
24-69:⚠️ Potential issue | 🟡 MinorType definition inconsistency:
tofield shown as required in guide but optional in API reference.The inline type definitions in this guide show
to: stringas a required field (lines 33 and 56), but the actual type definitions in the API reference docs (ToOptionsType.md and ToMaskOptionsType.md) showto?: ValidRoutePath | stringas optional. This discrepancy may confuse users about whethertois truly required.Additionally, maintaining two versions of the
ToMaskOptionsdefinition (here and in ToMaskOptionsType.md) creates potential for drift if one is updated without the other.Suggestions to resolve
Option 1 (Recommended): Update the guide to show
toas optional to match the API reference:type ToOptions< TRouteTree extends AnyRoute = AnyRoute, TFrom extends RoutePaths<TRouteTree> | string = string, TTo extends string = '', > = { from?: string - to: string + to?: stringAnd similarly for
ToMaskOptions:type ToMaskOptions<TRouteTree extends AnyRoute = AnyRoute> = { from?: string - to: string + to?: stringOption 2: If
tois meant to be required in practice, update the API reference types to reflect this and add a comment explaining when it can be omitted.Option 3: Add a comment in the guide explaining that
tois shown as required here for typical usage but is technically optional in the type system.
closes #6446
This PR improves the docs around route masking and navigation options, in response to #6446.
router.navigate()already supports masked navigation, but that support was not easy to find in the docs because mask was not explicitly surfaced in docs.Changes
Summary by CodeRabbit