-
Notifications
You must be signed in to change notification settings - Fork 6
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
Documentation site page #6861
Documentation site page #6861
Conversation
…in BasePageContentBlcok
WalkthroughThe changes introduce a new documentation feature within the application. This includes the addition of a dedicated documentation page component, updates to the GraphQL configuration schema, and routing modifications to support the new documentation path. A new translation entry for documentation has also been added, enhancing internationalization support. Overall, these updates create a structured way to access and display documentation content within the application. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router
participant DocumentationPage
User->>Router: Navigate to /docs
Router->>DocumentationPage: Load DocumentationPage
DocumentationPage->>User: Render documentation content
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: 2
🧹 Outside diff range and nitpick comments (6)
src/domain/platform/config/configuration.ts (1)
28-28
: LGTM! Consider adding a comment for clarity.The addition of the
documentation
property to thelocations
object in theConfiguration
interface aligns well with the PR objectives. It provides a way to store the documentation URL or path, which is essential for the iframe implementation described in the PR summary.Consider adding a brief comment above this property to describe its purpose, similar to:
// URL or path to the documentation site documentation: string;This would enhance code readability and provide context for other developers.
src/domain/documentation/DocumentationPage.tsx (2)
12-16
: Component definition and hooks usage look good, with a minor suggestion.The component is well-defined, and the hooks are used appropriately. However, consider handling the case where the URL doesn't contain the
/docs/
path segment.Consider adding a fallback for cases where the URL doesn't contain the
/docs/
path:- const docsInternalPath = pathname.split(`/${TopLevelRoutePath.Docs}/`)[1] ?? ''; + const pathParts = pathname.split(`/${TopLevelRoutePath.Docs}/`); + const docsInternalPath = pathParts.length > 1 ? pathParts[1] : '';This ensures
docsInternalPath
is always a string, even if the URL structure is unexpected.
20-38
: Component rendering looks good, with a suggestion for enhancement.The component structure and iframe implementation align well with the PR objectives and application conventions. The use of translations and accessibility considerations (iframe title) are commendable.
Consider adding a loading state or error handling for the iframe:
<Gutters height={'100vh'}> + {src ? ( <iframe src={src} title={t('pages.documentation.title')} style={{ width: '100%', height: '100%', border: 'none' }} + onLoad={() => {/* Handle successful load */}} + onError={() => {/* Handle load error */}} /> + ) : ( + <div>Loading documentation...</div> + )} </Gutters>This would improve user experience by providing feedback during loading or in case of errors.
src/main/routing/TopLevelRoutes.tsx (2)
35-35
: LGTM: Lazy loading of DocumentationPage.The use of React.lazy for DocumentationPage is a good practice for code splitting and improving initial load time.
Consider adding a type annotation for consistency with other lazy-loaded components:
const DocumentationPage = React.lazy(() => import('../../domain/documentation/DocumentationPage')) as React.LazyExoticComponent<React.ComponentType<any>>;
157-164
: LGTM: New route for DocumentationPage.The implementation of the new route for DocumentationPage is well-structured and correctly uses Suspense for the lazy-loaded component.
For consistency with other routes in this file, consider wrapping the DocumentationPage with a WithApmTransaction component:
<Route path={`${TopLevelRoutePath.Docs}/*`} element={ <WithApmTransaction path={TopLevelRoutePath.Docs}> <Suspense fallback={<Loading />}> <DocumentationPage /> </Suspense> </WithApmTransaction> } />This ensures that the documentation page is also tracked in your Application Performance Monitoring system.
src/core/i18n/en/translation.en.json (1)
2689-2691
: LGTM! Consider adding more keys for future expansion.The addition of the "documentation" section looks good. It's correctly placed within the "pages" object and follows the existing formatting conventions. For future-proofing, you might want to consider adding more keys that could be useful for the documentation page, such as a subtitle or description.
You could expand the section like this:
"documentation": { - "title": "Documentation" + "title": "Documentation", + "subtitle": "Explore our comprehensive guides", + "description": "Find detailed information about using our platform" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
src/core/apollo/generated/apollo-helpers.ts
is excluded by!**/generated/**
src/core/apollo/generated/apollo-hooks.ts
is excluded by!**/generated/**
src/core/apollo/generated/graphql-schema.ts
is excluded by!**/generated/**
📒 Files selected for processing (6)
- src/core/i18n/en/translation.en.json (1 hunks)
- src/domain/documentation/DocumentationPage.tsx (1 hunks)
- src/domain/platform/config/configuration.graphql (1 hunks)
- src/domain/platform/config/configuration.ts (1 hunks)
- src/main/routing/TopLevelRoutePath.ts (1 hunks)
- src/main/routing/TopLevelRoutes.tsx (3 hunks)
🔇 Additional comments (7)
src/main/routing/TopLevelRoutePath.ts (1)
16-16
: LGTM! The newDocs
route aligns with the PR objectives.The addition of
Docs = 'docs'
to theTopLevelRoutePath
enum is correct and consistent with the existing entries. This change supports the PR objective of creating a simple page for the documentation site, accessible at[env domain]/docs
.A few points to consider:
- The placement of the new entry is appropriate, maintaining alphabetical order within the enum.
- The
reservedTopLevelRoutePaths
constant will automatically include this new route, which is the desired behavior.- Ensure that corresponding changes have been made in the routing configuration to handle this new path (this should be verified in other files).
To confirm that the routing for the new docs page has been properly set up, you can run the following script:
This script will help verify that the necessary routing configurations and component imports for the new docs page are in place.
src/domain/platform/config/configuration.graphql (1)
48-48
: LGTM! The addition of thedocumentation
field is appropriate and aligns with the PR objectives.The new
documentation
field in thelocations
object of theConfiguration
fragment is a suitable addition. It provides a way to query the documentation URL through the GraphQL API, which can be used to set the iframe source as described in the PR objectives.To ensure consistency, let's verify if this change is reflected in other parts of the codebase:
Please review the output of these commands to ensure that all necessary parts of the codebase are updated to include the new
documentation
field.src/domain/documentation/DocumentationPage.tsx (3)
1-10
: LGTM: Imports are well-organized and relevant.The import statements are concise, follow a consistent style, and include all necessary components and utilities for the
DocumentationPage
component.
40-40
: LGTM: Component export is correct.The default export of the
DocumentationPage
component follows React best practices.
1-40
: Overall implementation successfully achieves PR objectives.The
DocumentationPage
component effectively implements the requirement of loading the stand-alone documentation site within an iframe. The code is well-structured, follows React best practices, and integrates well with the existing application architecture.Key points:
- Correct use of layout components and hooks.
- Proper integration with the application's routing and configuration.
- Implementation of internationalization.
While the core functionality is sound, consider implementing the suggested improvements for enhanced robustness and user experience:
- Handle edge cases in URL parsing.
- Add safeguards for potential configuration issues.
- Implement loading state and error handling for the iframe.
These enhancements will further improve the component's reliability and user experience.
src/main/routing/TopLevelRoutes.tsx (2)
33-33
: LGTM: Import statement for Loading component.The addition of the Loading component import is appropriate. It will be used as a fallback for the lazy-loaded DocumentationPage.
Line range hint
1-238
: Summary: Well-implemented documentation route addition.The changes to introduce the new documentation route are well-structured and align with the existing routing implementation. The use of lazy loading and Suspense is a good practice for optimizing performance. The placement of the new route within the existing structure is appropriate.
A few minor suggestions have been made for consistency and improved monitoring, but overall, the implementation is solid and achieves the goal of integrating the documentation page into the application.
As part of the new documentation initiative: https://github.com/alkem-io/documentation
Relates to: alkem-io/documentation#1
Depends on: alkem-io/server#4567
this PR provides a simple page for loading the stand-alone documentation site under an iframe.
The documentation can be accessed under [env domain]/docs. The iframe is with the source [env domain]/documentation*.
e.g.
https://dev-alkem.io/docs
which would load internally an iframe underhttps://dev-alkem.io/documentation
Summary by CodeRabbit
New Features
Bug Fixes
Documentation