-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
Added docs for the new Web Navigation API #822
Conversation
WalkthroughThe changes in this pull request involve updates to the documentation for the Web Browser History API and the introduction of the new Web Navigation API. The Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
docs/navigation/web-navigation.md (5)
3-5
: Consider enhancing the experimental warning.While the warning is good, consider adding more context about potential API changes and migration recommendations.
!!!warning - This API is experimental, available since version `3.3.0-alpha01`. + This API is experimental, available since version `3.3.0-alpha01`. As an experimental API, + it may undergo breaking changes in future releases. Please provide feedback and report any + issues you encounter to help stabilize the API.
96-96
: Enhance TODO placeholders with examples.The TODOs for initialization using
deepLinkUrl
would be more helpful with basic examples or guidelines.Consider adding comments like:
- initialStack = { TODO("Use the deepLinkUrl parameter to initialize the stack") }, + initialStack = { + // Example: Parse the deepLinkUrl to determine initial navigation state + // deepLinkUrl?.removePrefix("/") ?: "home" -> Config.Home + TODO("Use the deepLinkUrl parameter to initialize the stack") + },Also applies to: 152-152, 206-206
28-28
: Add clarification about nested navigation limitations.The statement about nested navigation could be clearer about the implications and any potential workarounds.
-Only one instance of the `WebNavigation` controller is allowed per component. The new API also supports nested navigation, there can only be up to one (zero or one) child `WebNavigationOwner` at a time. +Only one instance of the `WebNavigation` controller is allowed per component. While the API supports nested navigation, each component can have at most one child `WebNavigationOwner` at any time. This ensures proper coordination between parent and child navigation states.
34-34
: Consider adding error handling section.The documentation would benefit from a section about handling navigation errors and edge cases.
Consider adding a new section that covers:
- Handling invalid URLs or deep links
- Managing navigation state conflicts
- Error recovery strategies
- Browser compatibility considerations
233-233
: Enhance the example section.The reference to the main sample could be more helpful with specific file links and key points to look for.
-Please refer to the [main sample](https://github.com/arkivanov/Decompose/tree/master/sample) for a complete example of using the Web Navigation API. +Please refer to the [main sample](https://github.com/arkivanov/Decompose/tree/master/sample) for a complete example of using the Web Navigation API. Key implementation examples can be found in: +- `sample/app-js/src/jsMain/kotlin/com/arkivanov/sample/app/root/DefaultRootComponent.kt` - Root component setup +- `sample/app-js/src/jsMain/kotlin/com/arkivanov/sample/app/App.kt` - Web Navigation initialization
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
docs/navigation/stack/browser-history.md
(1 hunks)docs/navigation/web-navigation.md
(1 hunks)mkdocs.yml
(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.35.0)
docs/navigation/web-navigation.md
49-49: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
60-60: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
75-75: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
121-121: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
131-131: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
175-175: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
185-185: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
🔇 Additional comments (2)
docs/navigation/stack/browser-history.md (1)
3-5
: LGTM! Clear deprecation notice.
The warning effectively communicates the API's future removal and properly directs users to the new Web Navigation API documentation.
mkdocs.yml (1)
52-52
: LGTM! Well-placed navigation entry.
The new Web Navigation entry is properly positioned within the Navigation section, maintaining the documentation's logical structure.
663f9df
to
197f2ff
Compare
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
docs/navigation/web-navigation.md (5)
3-6
: Enhance version information in the warning sectionConsider adding more specific information about:
- The expected stability timeline
- Compatibility with the old API during transition
- Minimum version requirements for different features
Would you like me to propose an enhanced warning section?
28-28
: Add clarification about WebNavigation constraintsThe statement about navigation constraints could be clearer. Consider adding:
- Examples of incorrect configurations
- Explanation of why these constraints exist
- Diagrams showing valid/invalid nested navigation setups
Would you like me to propose expanded documentation for this section?
38-39
: Expand deployment configuration sectionThe catch-all strategy section could benefit from:
- Examples for common deployment platforms (Nginx, Apache, etc.)
- Troubleshooting guide for common issues
- Security considerations for different configurations
Would you like me to provide examples for different deployment scenarios?
96-96
: Enhance implementation guidance in examplesThe TODOs would be more helpful with:
- Concrete examples of typical implementations
- Best practices for each customization point
- Error handling scenarios
Would you like me to provide example implementations for these TODOs?
Also applies to: 107-109
231-233
: Enhance sample reference sectionConsider expanding this section with:
- Overview of what the sample demonstrates
- Key implementation patterns to observe
- Common pitfalls the sample helps avoid
Would you like me to propose an expanded sample reference section?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
docs/navigation/stack/browser-history.md
(1 hunks)docs/navigation/web-navigation.md
(1 hunks)mkdocs.yml
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- mkdocs.yml
- docs/navigation/stack/browser-history.md
🧰 Additional context used
🪛 Markdownlint (0.35.0)
docs/navigation/web-navigation.md
49-49: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
60-60: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
75-75: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
121-121: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
131-131: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
175-175: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
185-185: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
197f2ff
to
fdd9293
Compare
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.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
docs/navigation/web-navigation.md (4)
13-13
: Improve sentence clarityThe sentence structure could be clearer. Consider revising to:
"TheChild Slot
navigation model is currently not supported. It will likely require additional API changes and might be implemented in the future."🧰 Tools
🪛 LanguageTool
[uncategorized] ~13-~13: A determiner appears to be missing. Consider inserting it.
Context: ... likely require additional API changes, might be implemented in the future. ## WebNa...(AI_EN_LECTOR_MISSING_DETERMINER)
28-28
: Consider adding error handling informationThe documentation mentions the constraint of "only one instance per component" but doesn't specify what happens if this constraint is violated. Consider adding information about error handling or validation mechanisms.
38-39
: Add production deployment configuration examplesWhile the development configuration with webpack-dev-server is well explained, consider adding examples or guidance for production deployment configurations (e.g., nginx, Apache, or other common web servers).
231-232
: Enhance sample code referenceConsider adding more specific guidance about what aspects of the Web Navigation API implementation to look for in the main sample, such as:
- Deep linking implementation examples
- Navigation state management
- Integration with different navigation models
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
docs/navigation/stack/browser-history.md
(1 hunks)docs/navigation/web-navigation.md
(1 hunks)mkdocs.yml
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- mkdocs.yml
- docs/navigation/stack/browser-history.md
🧰 Additional context used
🪛 LanguageTool
docs/navigation/web-navigation.md
[uncategorized] ~13-~13: A determiner appears to be missing. Consider inserting it.
Context: ... likely require additional API changes, might be implemented in the future. ## WebNa...
(AI_EN_LECTOR_MISSING_DETERMINER)
🪛 Markdownlint (0.35.0)
docs/navigation/web-navigation.md
49-49: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
60-60: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
75-75: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
121-121: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
131-131: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
175-175: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
185-185: Expected: indented; Actual: fenced
Code block style
(MD046, code-block-style)
Summary by CodeRabbit
New Features
Documentation
browser-history.md
document for clarity and added warnings about the API's future.