Feat/carousel #485
Feat/carousel #485
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR introduces a carousel-based related articles component with internationalization support in English and Polish. It adds the ngx-owl-carousel-o library dependency, integrates carousel styling, and updates the avatar component with explicit pixel-based size bindings. Changes
Sequence DiagramsequenceDiagram
participant Component as RelatedArticles<br/>Component
participant Store as Article Store
participant Carousel as ngx-owl-carousel-o
participant i18n as Transloco i18n
Component->>Store: fetch related articles (ngOnInit)
activate Store
Store-->>Component: articles$ observable
deactivate Store
Component->>Carousel: initialize carousel with<br/>customOptions
activate Carousel
Carousel->>Carousel: render slides from articles
Carousel-->>Component: carousel ready
deactivate Carousel
Component->>i18n: load relatedArticles keys<br/>(title, previousSlide, nextSlide)
activate i18n
i18n-->>Component: translated strings
deactivate i18n
rect rgb(200, 220, 240)
Note over Component,Carousel: User interaction
Component->>Carousel: next/previous button click
Carousel->>Carousel: animate to next/previous slide
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Most changes are straightforward additions: i18n entries follow established patterns, styling imports are simple, and the avatar binding is a direct style addition. The component refactoring introduces carousel integration with clear structure and dependencies, but remains cohesive and well-defined. Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
PR is detected, will deploy to dev environment |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a carousel feature for displaying related articles on the blog, replacing the static grid layout with an interactive carousel component. It introduces the ngx-owl-carousel-o library to enable smooth navigation through related articles with responsive breakpoints and accessibility features.
Key changes:
- Addition of ngx-owl-carousel-o carousel library with custom navigation controls
- Implementation of internationalization for carousel navigation labels and titles
- Enhancement of avatar component sizing capabilities
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds ngx-owl-carousel-o library dependency |
| libs/blog/shared/ui-avatar/src/lib/avatar.component.html | Adds dynamic sizing support for avatar component |
| libs/blog/articles/feature-related-articles/src/lib/related-articles.component.ts | Replaces grid layout with carousel implementation including custom navigation buttons and accessibility features |
| apps/blog/src/styles.scss | Imports carousel library styles |
| apps/blog/src/assets/i18n/pl.json | Adds Polish translations for carousel navigation |
| apps/blog/src/assets/i18n/en.json | Adds English translations for carousel navigation |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| customOptions: OwlOptions = { | ||
| loop: true, | ||
| mouseDrag: false, | ||
| touchDrag: true, | ||
| pullDrag: true, | ||
| dots: true, | ||
| margin: 24, | ||
| navSpeed: 700, | ||
| navText: ['', ''], | ||
| responsive: { | ||
| // Keep in mind these breakpoints refer to container width, not the viewport width | ||
| 0: { | ||
| items: 1, | ||
| }, | ||
| 540: { | ||
| items: 2, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The carousel configuration object should be declared as readonly since it's never modified after initialization. This prevents accidental mutations and clearly communicates intent.
libs/blog/articles/feature-related-articles/src/lib/related-articles.component.ts
Show resolved
Hide resolved
| (click)="carousel.prev()" | ||
| size="small" | ||
| > | ||
| <span class="text-2xl">‹</span> |
There was a problem hiding this comment.
Using raw Unicode characters (‹ and ›) for navigation buttons may not be announced properly by screen readers. Consider using icon components with proper aria-hidden attributes or text alternatives instead.
|
Deployed to dev environment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/blog/articles/feature-related-articles/src/lib/related-articles.component.ts (1)
71-89: Consider disabling built-in navigation and review mouseDrag setting.Since custom navigation buttons are implemented, explicitly set
nav: falseto prevent potential conflicts with the library's default navigation.Additionally,
mouseDrag: falsedisables desktop drag functionality, which some users might expect. Consider whether this is intentional for the UX or if it should be enabled.Apply this diff to disable built-in navigation:
customOptions: OwlOptions = { loop: true, mouseDrag: false, touchDrag: true, pullDrag: true, dots: true, + nav: false, margin: 24, navSpeed: 700, navText: ['', ''],Additionally, consider enabling mouseDrag for desktop users:
- mouseDrag: false, + mouseDrag: true,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/blog/src/assets/i18n/en.json(1 hunks)apps/blog/src/assets/i18n/pl.json(1 hunks)apps/blog/src/styles.scss(1 hunks)libs/blog/articles/feature-related-articles/src/lib/related-articles.component.ts(1 hunks)libs/blog/shared/ui-avatar/src/lib/avatar.component.html(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/blog/articles/feature-related-articles/src/lib/related-articles.component.ts (2)
libs/blog/articles/feature-article/src/lib/article-details/article-details.component.ts (1)
Component(36-69)libs/blog/articles/data-access/src/lib/state/related-article.store.ts (1)
RelatedArticleListStore(25-60)
🔇 Additional comments (9)
libs/blog/shared/ui-avatar/src/lib/avatar.component.html (1)
3-4: LGTM! Explicit sizing improves layout consistency.The explicit width and height style bindings ensure the container matches the image dimensions, preventing potential layout shifts.
apps/blog/src/styles.scss (1)
5-6: LGTM! Carousel styling properly integrated.The imports for ngx-owl-carousel-o styles are correctly placed and will provide the necessary styling for the carousel component.
apps/blog/src/assets/i18n/en.json (1)
316-320: LGTM! Clear and appropriate translation keys.The new i18n keys for related articles provide proper labels for the carousel title and navigation controls, enhancing accessibility.
apps/blog/src/assets/i18n/pl.json (1)
319-323: LGTM! Consistent Polish translations.The Polish translations for related articles are properly structured and match the English keys, ensuring consistent i18n support.
libs/blog/articles/feature-related-articles/src/lib/related-articles.component.ts (4)
8-13: LGTM! Dependencies correctly imported.The new imports for TranslocoDirective, CarouselModule, OwlOptions, and ButtonComponent properly support the carousel-based i18n implementation.
19-30: LGTM! Strong accessibility implementation.The carousel implementation demonstrates good accessibility practices:
- Transloco integration for i18n support
role="list"on the carousel container- List items wrapped in
<li>elements- Padding added to prevent focus ring clipping
32-56: LGTM! Custom navigation with proper ARIA labels.The custom navigation buttons with ARIA labels provide better accessibility than default carousel controls. The positioning for large viewports is appropriate.
69-96: Verify YARPP plugin configuration for article count.As noted in the PR description, the YARPP plugin configuration needs to be updated to return four related articles instead of two for proper carousel presentation.
Verify that the backend/YARPP plugin is configured to return the expected number of related articles. This can be confirmed by checking the API response or configuration settings.
package.json (1)
60-60: Package version verified as valid with no security vulnerabilities.ngx-owl-carousel-o version 20.0.1 is the latest version on NPM, and no security advisories were found for this package.
|
PR is detected, will deploy to dev environment |
|
Deployed to dev environment |
Adds a POC for a related articles wrapper. Currently, it renders 2 elements for each related article returned by the backend for presentation purposes. According to my findings, we need to update the YARPP plugin configuration to change the number of returned related articles from 2 to 4.
I've utilized the ngx-owl-carousel-o library for implementing the carousel. I'm leaving the final decision on including it in the package.json on the main branch for further consideration.
Additionally, this PR adds a missing translation for the Related Articles title and ensures proper accessibility for the carousel navigation.
Summary by CodeRabbit
New Features
Improvements