feat(web): Stepper on subarticles #16541
Conversation
Datadog ReportAll test runs ✅ 81 Total Test Services: 0 Failed, 78 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
RunarVestmann
added
the
deprecated:automerge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
apps/web/screens/queries/Article.ts (1)
136-150: LGTM! Consider documenting the stepper configuration.The stepper field structure is well-organized and follows GraphQL best practices. However, please consider adding documentation comments to explain:
- The purpose and structure of the
configfield at both stepper and step levels- The possible values for the
stepTypefieldapps/web/screens/Article/Article.tsx (1)
Line range hint
807-812: Maintain consistent heading levels for better accessibility.Rendering the sub-article title as an
h2may affect the document outline and accessibility. Ensure that heading levels (h1,h2,h3, etc.) are used sequentially to provide a clear structure for users and assistive technologies.Consider adjusting the heading levels based on the context:
- <Text variant="h2" as="h2" paddingTop={7}> + <Text variant="h3" as="h3" paddingTop={7}>Or adjust surrounding headings to maintain a consistent hierarchy.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
libs/cms/src/lib/generated/contentfulTypes.d.tsis excluded by!**/generated/**
📒 Files selected for processing (4)
- apps/web/screens/Article/Article.tsx (9 hunks)
- apps/web/screens/queries/Article.ts (1 hunks)
- libs/cms/src/lib/models/subArticle.model.ts (3 hunks)
- libs/island-ui/core/src/lib/Breadcrumbs/Breadcrumbs.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/web/screens/Article/Article.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/queries/Article.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/cms/src/lib/models/subArticle.model.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/core/src/lib/Breadcrumbs/Breadcrumbs.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (7)
libs/cms/src/lib/models/subArticle.model.ts (4)
8-8: LGTM: Clean import declarationThe import follows the established pattern and properly imports both the type and mapping function.
33-34: LGTM: Well-structured field declarationThe field declaration follows best practices:
- Uses
@CacheFielddecorator consistently with other fields- Proper TypeScript nullable type definition
- Maintains the same pattern as other optional fields in the class
59-59: LGTM: Consistent mapping implementationThe mapping implementation:
- Follows the null-safe pattern used throughout the file
- Uses the imported mapStepper function consistently
- Maintains the same structure as other optional field mappings
Line range hint
1-61: Verify the stepper integration across the applicationSince this is a model change in a shared library, we should verify its usage across the application.
✅ Verification successful
Let me generate new shell scripts to better verify the stepper integration.
The previous attempt failed because of file type restrictions. Let's try a broader search to find all relevant usages.
Based on the comprehensive search results, I can now provide a final response about the stepper integration:
Stepper integration is well-established and properly integrated across the application
The stepper field in SubArticle model is properly integrated and used throughout the application:
It's used in multiple core models:
SubArticlemodel (libs/cms/src/lib/models/subArticle.model.ts)Articlemodel (libs/cms/src/lib/models/article.model.ts)ProjectPagemodel (libs/cms/src/lib/models/projectPage.model.ts)The integration is complete with:
- GraphQL schema definitions in
libs/clients/cms/schema.graphql- Proper imports and mapping functions in all relevant models
- Full UI implementation in
apps/web/components/Stepper- Usage in article and project screens (
apps/web/screens/Articleandapps/web/screens/Project)The stepper field is properly consumed by the application with all necessary type definitions, mappings, and UI components in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the stepper field usage in GraphQL queries and components # Check for GraphQL queries using the stepper field echo "Checking GraphQL queries using the stepper field..." rg -t graphql "stepper" -A 5 # Check for React components that might be consuming the stepper field echo "Checking React components using the stepper field..." rg -t typescript -t tsx "stepper.*SubArticle" -A 5Length of output: 376
Script:
#!/bin/bash # Search for stepper-related code across all files echo "Searching for stepper field usage..." rg "stepper" -A 5 # Search for SubArticle query fragments echo "Searching for SubArticle query fragments..." rg "fragment.*SubArticle" -A 10 # Search for stepper model imports echo "Searching for stepper model imports..." rg "import.*stepper\.model" # Search for files that might be using the stepper field echo "Searching for potential stepper consumers..." fd -e ts -e tsx -e graphql . | xargs rg "stepper"Length of output: 71643
libs/island-ui/core/src/lib/Breadcrumbs/Breadcrumbs.tsx (1)
Line range hint
1-99: Well-structured reusable component.The component follows best practices for reusability and maintainability:
- Proper TypeScript types and interfaces
- Flexible customization through props
- Clean separation of concerns
- Effective tree-shaking support
apps/web/screens/Article/Article.tsx (2)
768-777: Ensure 'ArticleNavigation' renders correctly in all views.When
inStepperViewisfalse, you conditionally renderArticleNavigation. Verify that this component displays appropriately across different devices and screen sizes, and that the navigation links function as intended.
427-477: Verify breadcrumb items for accurate navigation paths.In the
breadcrumbItemsconstruction, ensure that the URLs and titles are correctly generated, especially wheninStepperViewistrue. This will help users navigate accurately through articles and sub-articles.Run the following script to check the breadcrumb items:
✅ Verification successful
Let me try a different approach to verify the breadcrumb implementation.
Let's analyze the implementation details and usage patterns to verify the breadcrumb construction.
Let me try one more search to understand the BreadCrumbItem type definition and its usage in the Breadcrumbs component.
This will help us verify the breadcrumb structure and navigation paths.
Based on the gathered information, I can now provide a final response about the breadcrumb implementation.
Breadcrumb implementation is correctly handling navigation paths
The breadcrumb implementation in Article.tsx follows the correct structure and best practices:
The BreadCrumbItem interface is properly defined with all necessary fields:
title: string (display text)href: string (optional direct URL)slug: string[] (for dynamic path construction)typename: string (for type-specific routing)isTag: boolean (for special rendering)The breadcrumb construction logic correctly handles both scenarios:
- In stepper view: Shows article and sub-article hierarchy
- In regular view: Shows full path including homepage, category, and group tags
The slug handling is consistent with the application's routing patterns, using arrays for multi-segment paths and proper concatenation for group slugs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify breadcrumb items and their URLs. # Test: Output breadcrumb items. Expect: Correct titles and URLs. ast-grep --pattern $'breadcrumbItems = $_' --typescript "apps/web/screens/Article/Article.tsx"Length of output: 291
Script:
#!/bin/bash # Search for breadcrumb-related code and imports rg -A 5 "BreadCrumbItem" apps/web/screens/Article/Article.tsx # Look for any tests related to breadcrumbs fd -e test.tsx -e spec.tsx -X rg -l "breadcrumb" # Check for BreadCrumbItem type definition rg -A 5 "type BreadCrumbItem" # Look for any usage of breadcrumbs in components rg -A 2 "breadcrumbItems"Length of output: 16511
Script:
#!/bin/bash # Search for BreadCrumbItem type definition and Breadcrumbs component usage rg -A 10 "interface BreadCrumbItem|type BreadCrumbItem" # Look for Breadcrumbs component implementation fd -e tsx -e ts -X rg -l "export.*Breadcrumbs" # Check how slugs are used in breadcrumb navigation rg -A 3 "slug.*=.*\[.*\]"Length of output: 7891
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
libs/cms/src/lib/models/subArticle.model.ts (1)
59-59: LGTM: Mapping implementation is type-safeThe stepper mapping follows the established pattern and includes proper null checking. However, since this is a new field, we should ensure it's covered by tests.
Consider adding test cases for:
- Mapping with stepper field present
- Mapping with stepper field undefined
- Mapping with stepper field null
Would you like me to help generate these test cases?
libs/island-ui/core/src/lib/Breadcrumbs/Breadcrumbs.tsx (1)
62-66: Consider adding aria-label to improve accessibility.While the breadcrumb navigation is already accessible, you could enhance it by adding an aria-label to each item to provide more context.
<Box key={`${item.title}-${index}`} display={'inlineFlex'} alignItems={'center'} + aria-label={`Breadcrumb ${index + 1} of ${visibleItems.length}: ${item.title}`} >apps/web/screens/queries/Article.ts (1)
136-150: Consider adding TypeScript types for stepper-related fields.The stepper structure is well-organized, but would benefit from explicit TypeScript types for better type safety and developer experience.
Consider creating the following types:
interface StepperConfig { // Define configuration options } interface Step { id: string title: string slug: string stepType: string subtitle: AllSlices config: Record<string, unknown> } interface Stepper { id: string title: string steps: Step[] config: StepperConfig }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
libs/cms/src/lib/generated/contentfulTypes.d.tsis excluded by!**/generated/**
📒 Files selected for processing (5)
- apps/web/screens/Article/Article.tsx (9 hunks)
- apps/web/screens/queries/Article.ts (1 hunks)
- libs/cms/src/lib/models/subArticle.model.ts (3 hunks)
- libs/cms/src/lib/search/importers/manualChapterItem.service.ts (2 hunks)
- libs/island-ui/core/src/lib/Breadcrumbs/Breadcrumbs.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/web/screens/Article/Article.tsx (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/web/screens/queries/Article.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/cms/src/lib/models/subArticle.model.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/cms/src/lib/search/importers/manualChapterItem.service.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/island-ui/core/src/lib/Breadcrumbs/Breadcrumbs.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (8)
libs/cms/src/lib/models/subArticle.model.ts (2)
8-8: LGTM: Import follows established patternsThe import statement follows the existing pattern and properly imports both the mapping function and type definition.
32-34: LGTM: Field declaration follows best practicesThe stepper field declaration:
- Uses proper TypeScript optional types
- Follows the established pattern with @CacheField decorator
- Maintains consistency with similar optional fields in the model
Let's verify the Stepper type definition exists:
✅ Verification successful
Let me try a different search strategy to locate the Stepper type definition.
This comprehensive search should help us locate the Stepper type definition or understand how it's being imported and used in the codebase.
Let me check the stepper.model.ts file since it appears to be the source of the Stepper type used in subArticle.model.ts.
Stepper type is properly defined and used correctly
The verification confirms that:
- The
Stepperclass is properly defined inlibs/cms/src/lib/models/stepper.model.ts- It's correctly imported in
subArticle.model.ts- The type has all necessary fields and decorators for GraphQL schema generation
- It maps to a valid Contentful interface
IStepperThe optional stepper field in
subArticle.model.tsis therefore correctly typed and follows the established patterns in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the Stepper type definition ast-grep --pattern 'export class Stepper { $$$ }'Length of output: 53
Script:
#!/bin/bash # Search for Stepper type definition using multiple approaches echo "=== Searching for Stepper class definition ===" ast-grep --pattern 'class Stepper' echo -e "\n=== Searching for Stepper type/interface definition ===" ast-grep --pattern 'type Stepper' ast-grep --pattern 'interface Stepper' echo -e "\n=== Searching for Stepper import statements ===" rg "import.*Stepper" -A 1 echo -e "\n=== Searching for files containing Stepper ===" fd -t f "Stepper"Length of output: 6961
Script:
#!/bin/bash # Check the stepper model definition echo "=== Checking stepper.model.ts content ===" cat libs/cms/src/lib/models/stepper.model.ts echo -e "\n=== Checking the interface definition from contentfulTypes ===" rg "interface IStepper" -A 10Length of output: 2561
libs/island-ui/core/src/lib/Breadcrumbs/Breadcrumbs.tsx (1)
62-66: LGTM: Key implementation addresses focus retention issues.The combination of title and index creates a more stable key that should resolve the focus retention problems mentioned in the PR objectives. This is a good improvement over using just the index.
apps/web/screens/queries/Article.ts (1)
144-146: Verify AllSlices fragment compatibility.The
subtitlefield uses theAllSlicesfragment. Ensure that all slice types are appropriate for the subtitle context.libs/cms/src/lib/search/importers/manualChapterItem.service.ts (1)
25-25: LGTM: Improved error handling with optional chainingThe addition of optional chaining operator (?.) is a good defensive programming practice that prevents runtime errors when
chaptersis undefined.apps/web/screens/Article/Article.tsx (3)
Line range hint
323-327: Consider specifying explicit types instead of 'any'In the
ArticlePropsinterface, thestepOptionsFromNamespaceproperty usesanyinRecord<string, any>[]. Usinganydiminishes type safety and can lead to potential errors. Defining explicit types for the expected data structure enhances type checking and maintainability.
422-424: Ensure 'query.stepper' comparison accounts for boolean valuesIn the
inStepperViewcomputation, you're comparingquery.stepperto the string'true'. Ifquery.steppercan also be a boolean, consider adjusting the comparison to handle both string and boolean representations oftrueto prevent unexpected behavior.
705-709: Avoid unnecessary type assertion with 'as StepperSchema'In the
Steppercomponent, you're using a type assertion to cast(subArticle ? subArticle.stepper : article?.stepper)toStepperSchema. Refining the types ofarticle.stepperandsubArticle.stepperto matchStepperSchemacan eliminate the need for type assertions and improve type safety.
Stepper on subarticles
What
Screenshots / Gifs
Screen.Recording.2024-10-24.at.10.55.38.mov
Before breadcrumb key change
Screen.Recording.2024-10-24.at.11.13.41.mov
After breadcrumb key change
Screen.Recording.2024-10-24.at.11.14.13.mov
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
ArticleScreencomponent for better readability and functionality.