feat(main): newsletter & top banners #380
Conversation
Deploying angular-love-client with
|
| Latest commit: |
87d4b61
|
| Status: | 🚫 Build failed. |
majahendzel-va
force-pushed
the
feat/multiple-banner-logic
branch
from
927dee7 to
5a7f82f
Compare
| getVisibleBanners() { | ||
| return this._http.get<Banners>(`${this._apiBaseUrl}/banners`); | ||
| } |
There was a problem hiding this comment.
can we host this data as a json asset? it would be created in the build time as a part of the process in apps/blog/scripts/build-routes.mjs. This resource can be received via http client, similarly to libs/blog/articles/data-access/src/lib/guards/article-exists.guard.ts.
With current implementation we fetch banners from BFF for every available page. Basically it adds an extra request during prerendering. If we prerender 500 pages, it ends up with BFF 1000 request, where 50% is the same banners request.
There was a problem hiding this comment.
this pr resolves the issue #415
WalkthroughThis update refactors the banner system to support multiple banner types—slider, top, and card banners—with device-specific images and dynamic display controls. It introduces new DTOs, type definitions, UI components, and service methods, shifting banner data fetching to a remote API and updating state management and component integration accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as UI Component
participant Store as AdBannerStore
participant Service as AdBannerService
participant API as Remote Banner API
Component->>Store: getData()
Store->>Service: getVisibleBanners()
Service->>API: GET /banners
API-->>Service: Banners DTO
Service-->>Store: Banners
Store-->>Component: banners (slider, topBanner, cardBanner)
Component->>Component: Conditionally render banners based on presence
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
PR is detected, will deploy to dev environment |
|
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: 5
🧹 Nitpick comments (3)
libs/blog-contracts/banners/src/lib/banners.ts (1)
1-8: Consider documenting the optional propertiesThe
urlandaltproperties were changed to optional, which could affect existing code.Consider adding JSDoc comments to explain why these properties are optional:
export interface Slider { + /** Time in milliseconds to display each slide before transitioning to the next */ slideDisplayTimeMs: number; slides: { + /** Optional URL to the slide image - may be undefined if no image is available */ url?: string; + /** Optional alt text for accessibility - may be undefined if no alt text is provided */ alt?: string; + /** URL to navigate to when the slide is clicked */ navigateTo: string; }[]; }libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts (2)
26-26: Unused template reference variableThe template reference variable
#topBannerRefis defined but never used in the component. Consider removing it if it's not needed for future functionality.- <al-top-banner #topBannerRef /> + <al-top-banner />
75-86: Handle possible undefined values more safelyThe current implementation uses optional chaining but could still result in runtime errors if data is malformed.
protected readonly slides = computed<AdImageBanner[] | undefined>(() => - this.bannerStore.banners()?.slider?.slides.map((slide) => ({ + this.bannerStore.banners()?.slider?.slides?.map((slide) => ({ url: slide.url!, alt: slide.alt!, action: { type: 'url', url: slide.navigateTo, }, })), ); protected readonly msPerSlide = computed( - () => this.bannerStore.banners()?.slider?.slideDisplayTimeMs, + () => this.bannerStore.banners()?.slider?.slideDisplayTimeMs ?? 5000, // Provide a default value );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/blog/src/assets/icons/arrow-down.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
libs/blog-bff/banners/api/src/lib/dtos.ts(1 hunks)libs/blog-bff/banners/api/src/lib/mappers.ts(1 hunks)libs/blog-contracts/banners/src/lib/banners.ts(1 hunks)libs/blog/ad-banner/data-access/src/lib/infrastructure/ad-banner.service.ts(1 hunks)libs/blog/ad-banner/data-access/src/lib/state/ad-banner.store.ts(2 hunks)libs/blog/ad-banner/ui/src/index.ts(1 hunks)libs/blog/ad-banner/ui/src/lib/newsletter-banner/al-newsletter-banner.component.ts(1 hunks)libs/blog/articles/feature-article/src/lib/article-details/article-details.component.html(1 hunks)libs/blog/articles/feature-article/src/lib/article-details/article-details.component.ts(2 hunks)libs/blog/articles/feature-latest-articles/src/lib/feature-latest-articles/feature-latest-articles.component.html(1 hunks)libs/blog/articles/feature-latest-articles/src/lib/feature-latest-articles/feature-latest-articles.component.ts(3 hunks)libs/blog/articles/ui-article-card/src/lib/components/article-horizontal-card/article-horizontal-card.component.html(1 hunks)libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
libs/blog/ad-banner/ui/src/lib/newsletter-banner/al-newsletter-banner.component.ts (2)
libs/blog/articles/feature-latest-articles/src/lib/feature-latest-articles/feature-latest-articles.component.ts (1)
Component(32-94)libs/blog-contracts/banners/src/lib/banners.ts (1)
CardBanner(16-20)
libs/blog/ad-banner/data-access/src/lib/infrastructure/ad-banner.service.ts (1)
libs/blog-contracts/banners/src/lib/banners.ts (1)
Banners(22-26)
libs/blog/ad-banner/data-access/src/lib/state/ad-banner.store.ts (1)
libs/blog-contracts/banners/src/lib/banners.ts (1)
Banners(22-26)
libs/blog/articles/feature-latest-articles/src/lib/feature-latest-articles/feature-latest-articles.component.ts (1)
libs/blog/ad-banner/data-access/src/lib/state/ad-banner.store.ts (1)
AdBannerStore(19-40)
libs/blog/articles/feature-article/src/lib/article-details/article-details.component.ts (1)
libs/blog/ad-banner/data-access/src/lib/state/ad-banner.store.ts (1)
AdBannerStore(19-40)
libs/blog-bff/banners/api/src/lib/mappers.ts (2)
libs/blog-bff/banners/api/src/lib/dtos.ts (2)
WPBannerDto(1-21)WPBannerMediaDto(23-29)libs/blog-contracts/banners/src/lib/banners.ts (1)
Banners(22-26)
libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts (5)
libs/blog/ad-banner/ui/src/lib/top-banner.component.ts (1)
Component(3-20)libs/blog/layouts/ui-layouts/src/lib/header/header.component.ts (1)
Component(20-75)libs/blog/ad-banner/ui/src/lib/banner-carousel/al-banner-carousel.component.ts (1)
Component(7-36)libs/blog/ad-banner/data-access/src/lib/state/ad-banner.store.ts (1)
AdBannerStore(19-40)libs/blog/ad-banner/ui/src/lib/ad-image-banner/ad-image-banner-data.interface.ts (1)
AdImageBanner(1-5)
🔇 Additional comments (31)
libs/blog/ad-banner/ui/src/index.ts (1)
2-2: Exports updated to support new newsletter banner componentThe barrel file has been appropriately updated to export the new
al-newsletter-banner.componentand reorder exports to maintain logical grouping (data interface before component).Also applies to: 4-4
libs/blog/articles/ui-article-card/src/lib/components/article-horizontal-card/article-horizontal-card.component.html (1)
3-3: CSS class reordering looks goodThe reordering of CSS classes on the article element doesn't change functionality and maintains the same styling.
libs/blog/articles/feature-article/src/lib/article-details/article-details.component.html (1)
65-67: Direct banner check is more efficientThe conditional styles now directly check for the presence of topBanner instead of using a method call, which is more readable and efficient. The optional chaining operator is correctly used for null safety.
libs/blog/articles/feature-latest-articles/src/lib/feature-latest-articles/feature-latest-articles.component.html (1)
32-45: Well-implemented conditional newsletter banner renderingThe template now intelligently renders either the new newsletter banner component or falls back to the standard newsletter card based on the availability of banner data. The conditional syntax using
@if/elsewith theasalias is clean and follows Angular best practices.libs/blog/ad-banner/data-access/src/lib/infrastructure/ad-banner.service.ts (2)
4-5: Appropriate import substitution for the new banner structure.The import changes correctly reflect the transition from a single slider model to a broader
Bannersinterface that can include multiple banner types (slider, topBanner, cardBanner). AddingConfigServiceallows for dynamic API URL construction.
9-10: Good use of dependency injection pattern.Using
inject()for the ConfigService is aligned with Angular's recommended patterns, and extracting theapiBaseUrlas a private readonly property follows good encapsulation principles.libs/blog/articles/feature-article/src/lib/article-details/article-details.component.ts (3)
5-7: Good import update for dependency injection.Adding the
injectimport from@angular/coreis appropriate for the new injection pattern used in the component.
10-10: Proper import for store access.The import of
AdBannerStoreprovides access to the centralized banner data management, which is a good architectural approach.
61-61: Direct store injection improves state management.Replacing a local reactive boolean signal with direct injection of the
AdBannerStoreservice follows Angular best practices for state management. This change aligns with the broader refactoring of the banner system and provides more granular access to the available banner types.libs/blog/ad-banner/data-access/src/lib/state/ad-banner.store.ts (4)
7-7: Import correctly updated for new banner structure.The import has been updated from
SlidertoBannersto reflect the broader banner data model that includes multiple banner types.
11-13: Type definition properly updated for expanded banner model.The state type has been correctly updated from
{ slider: Slider | null }to{ banners: Banners | null }to accommodate the expanded banner model that includes multiple banner types (slider, topBanner, cardBanner).
15-17: Initial state correctly updated.The initial state has been appropriately updated to match the new state structure.
25-30: Service method call and state update properly aligned with new structure.The store now correctly calls
getVisibleBanners()instead ofgetBannerSlider()and updates the state with the newbannersproperty, aligning with the service changes and expanded banner model.libs/blog/ad-banner/ui/src/lib/newsletter-banner/al-newsletter-banner.component.ts (2)
6-9: Component declaration follows Angular best practices.Good use of OnPush change detection strategy and proper import of NgOptimizedImage for optimized image handling.
42-44: Input property defined correctly.Good use of
input.required<CardBanner>()to define a required input property with proper typing.libs/blog/articles/feature-latest-articles/src/lib/feature-latest-articles/feature-latest-articles.component.ts (4)
12-12: Import organization looks goodClean addition of the AdBannerStore import to support the new banner functionality.
19-19: Appropriate import for newsletter banner componentThe AlNewsletterBannerComponent import aligns with the new banner system.
44-49: Component imports updated correctlyAdded the necessary imports (CardContentDirective, AlNewsletterBannerComponent) and reordered GradientCardDirective, maintaining a clean imports section.
68-68: Good store injection approachCorrectly injecting the AdBannerStore using Angular's recommended inject pattern.
libs/blog-contracts/banners/src/lib/banners.ts (3)
10-14: New TopBanner interface looks goodThe TopBanner interface follows the same structure as the Slider's slide items, maintaining consistency.
16-20: New CardBanner interface looks goodThe CardBanner interface is consistent with the TopBanner interface, which is good for maintainability.
22-26: Well-structured Banners aggregation interfaceThe Banners interface effectively aggregates all banner types, making it easy to manage multiple banner types in a single object.
libs/blog-bff/banners/api/src/lib/dtos.ts (2)
4-10: Good banner type flag and device-specific image handlingThe DTO now properly supports different device images and has a clear flag to control visibility.
12-15: Top banner properties follow consistent patternThe top banner properties follow the same pattern as the slider with desktop/mobile versions and navigation URL.
libs/blog-bff/banners/api/src/lib/mappers.ts (2)
1-8: Updated import and return typeGood update to the import and return type to match the new Banners interface.
10-24: Slider mapping implemented correctlyThe conditional inclusion of the slider based on the display flag is well implemented.
libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts (5)
28-39: Verify the purpose of dual banner componentsThere seems to be redundancy with both
<al-top-banner>and the conditional<al-ad-image-banner>for top banners. Based on the overall refactoring, it appears you're transitioning to a dynamic banner system, but it's unclear if both components are intentionally needed.Also, consider using the nullish coalescing operator instead of non-null assertions for better safety:
@if (topBanner(); as topBanner) { <al-ad-image-banner [banner]="{ - url: topBanner.url!, - alt: topBanner.alt!, + url: topBanner.url ?? '', + alt: topBanner.alt ?? '', action: { type: 'url', - url: topBanner.navigateTo!, + url: topBanner.navigateTo ?? '', }, }" /> }
73-73: LGTM! Appropriate store namingThe rename from
sliderStoretobannerStorebetter reflects the expanded functionality for multiple banner types.
87-89: LGTM! Clean computed property implementationThe implementation of the
topBannercomputed property is clean and correctly uses optional chaining.
93-94: Address TODO commentThere's a comment indicating this is a temporary solution. Consider creating a ticket to track this technical debt or implement a more permanent solution.
The
adBannerVisiblecomputed property is hardcoded tofalse. Is this intentional or should it be derived from banner data?
114-116: LGTM! Simplified constructorThe constructor has been simplified to only fetch banner data, removing previous viewport scroll handling.
| getVisibleBanners() { | ||
| return this._http.get<Banners>(`${this._apiBaseUrl}/banners`); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider caching mechanism to address previous concerns about redundant requests.
The method has been appropriately renamed from getBannerSlider() to getVisibleBanners() to reflect its expanded purpose. The implementation is clean and focused.
However, I noticed in past review comments a concern about making redundant banner requests during prerendering. Consider implementing a caching mechanism or moving to a static JSON asset as previously suggested to optimize performance.
🏁 Script executed:
#!/bin/bash
# Check if there's any caching implementation for banner requests
echo "Searching for caching implementations in banner-related code..."
rg -A 5 -B 5 "cache|shareReplay|replaySubject" --iglob "**/*banner*/**"Length of output: 1094
🌐 Web query:
What are common strategies for reducing redundant API requests during Angular SSR/prerendering?
💡 Result:
During Angular SSR (Server-Side Rendering) or prerendering, it's common to face redundant API requests because Angular's rendering lifecycle might trigger multiple calls to the same data source. Here are some common strategies to reduce these redundant requests:
-
State Transfer with TransferState API
Angular provides aTransferStateAPI that allows you to store the results of API calls on the server and then re-use that data on the client without making the API call again.- During SSR, you fetch data and store it in
TransferState. - On the client side, you read from
TransferStatefirst. If data is available, skip the HTTP call.
This efficiently reduces redundant calls since the client can reuse server-fetched data.
- During SSR, you fetch data and store it in
-
Caching HTTP Requests
Use an HTTP caching mechanism on the server during SSR. You can implement operators on the HTTP service likeshareReplayto cache observable results so that simultaneous or repeated subscriptions don’t trigger multiple HTTP calls.- This is useful if multiple components/services request the same data.
- Combine with
TransferStatefor best results.
-
Pre-fetching Data at a Higher Level
Instead of each component making its own API call, lift the data fetching up to a parent or even to the route resolver.- Fetch data once per route during SSR.
- Pass data down to child components via @input bindings, reducing multiple API calls.
-
Avoid API Calls in ngOnInit or Constructor
Sometimes API calls placed in lifecycle hooks without coordination cause multiple calls. Refactor to use centralized services that handle data fetching with caching or transfer state. -
Using Route Resolvers
Use Angular route resolvers to fetch data before the route activates. During SSR, these fetches happen once per route transition, preventing redundant requests that happen due to component re-instantiations. -
Idempotent APIs and Request Deduplication Logic
If your backend APIs support idempotent fetches, you can implement deduplication logic on the client-side or at the HTTP interceptor level to cancel or merge multiple identical requests. -
Universal-aware Data Services
Design services that can detect if they are running on the server or client (usingisPlatformServer) to conditionally trigger API calls or fetch from caches. -
Single Source of Truth using State Management
Using NGRX or other state management libraries to keep a unified store. API data fetched once during SSR is hydrated into the store and accessed by components without triggering new calls.
Summary: The most effective and widely used approach is combining Angular's TransferState API to pass pre-fetched data from server to client and HTTP caching techniques like shareReplay to prevent duplicate calls during SSR. Route resolvers and centralized data fetching also contribute significantly to this goal.
Implement HTTP caching to prevent redundant banner requests during SSR
This service currently issues a fresh HTTP GET on every call:
- File: libs/blog/ad-banner/data-access/src/lib/infrastructure/ad-banner.service.ts
Method:getVisibleBanners()(lines 12–13)
To optimize performance during Angular Universal prerendering, consider one or more of the following:
• Use Angular’s TransferState API
– Fetch banners on the server, store them in TransferState, and read from it on the client to skip the extra HTTP call.
• Cache the Observable with shareReplay(1)
– Wrap the HTTP request in a shared ReplaySubject so multiple subscribers reuse the same response.
• Lift data fetching to a route resolver or parent service
– Fetch once per route and pass data down via @input, avoiding repeated calls in individual components.
• Pre-render a static banners JSON asset
– If banner data changes infrequently, bundle a JSON file in assets/ and load it instead of calling the API at runtime.
See Angular’s TransferState guide for details: https://angular.io/api/platform-browser/TransferState
| template: ` | ||
| <a | ||
| class="relative flex h-full items-center justify-center" | ||
| [href]="cardBanner().navigateTo!" | ||
| > | ||
| <aside class="absolute h-full w-full overflow-hidden rounded-lg"> | ||
| <img | ||
| tabindex="0" | ||
| role="button" | ||
| class="!relative object-cover shadow-inner blur-xl" | ||
| [attr.aria-label]="cardBanner()!.alt!" | ||
| [alt]="cardBanner().alt!" | ||
| [ngSrc]="cardBanner().url!" | ||
| fill | ||
| priority | ||
| /> | ||
| </aside> | ||
| <aside> | ||
| <img | ||
| tabindex="0" | ||
| role="button" | ||
| class="!relative" | ||
| [attr.aria-label]="cardBanner().alt!" | ||
| [alt]="cardBanner().alt!" | ||
| [ngSrc]="cardBanner().url!" | ||
| fill | ||
| priority | ||
| /> | ||
| </aside> | ||
| </a> | ||
| `, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve template safety and accessibility.
The template has several issues that should be addressed:
- Non-null assertions (
!) are used extensively, which could lead to runtime errors if properties are undefined. - Duplicate images are loaded for the same content, which is inefficient.
- The
role="button"on images inside an anchor tag is redundant and potentially confusing for screen readers.
Consider this safer and more efficient approach:
- template: `
- <a
- class="relative flex h-full items-center justify-center"
- [href]="cardBanner().navigateTo!"
- >
- <aside class="absolute h-full w-full overflow-hidden rounded-lg">
- <img
- tabindex="0"
- role="button"
- class="!relative object-cover shadow-inner blur-xl"
- [attr.aria-label]="cardBanner()!.alt!"
- [alt]="cardBanner().alt!"
- [ngSrc]="cardBanner().url!"
- fill
- priority
- />
- </aside>
- <aside>
- <img
- tabindex="0"
- role="button"
- class="!relative"
- [attr.aria-label]="cardBanner().alt!"
- [alt]="cardBanner().alt!"
- [ngSrc]="cardBanner().url!"
- fill
- priority
- />
- </aside>
- </a>
+ template: `
+ <a
+ class="relative flex h-full items-center justify-center"
+ [href]="cardBanner().navigateTo"
+ >
+ <aside class="absolute h-full w-full overflow-hidden rounded-lg">
+ <img
+ *ngIf="cardBanner().url"
+ class="!relative object-cover shadow-inner blur-xl"
+ [attr.aria-label]="cardBanner().alt || 'Newsletter banner'"
+ [alt]="cardBanner().alt || 'Newsletter banner'"
+ [ngSrc]="cardBanner().url"
+ fill
+ priority
+ />
+ </aside>
+ <aside>
+ <img
+ *ngIf="cardBanner().url"
+ class="!relative"
+ [attr.aria-label]="cardBanner().alt || 'Newsletter banner'"
+ [alt]="cardBanner().alt || 'Newsletter banner'"
+ [ngSrc]="cardBanner().url"
+ fill
+ priority
+ />
+ </aside>
+ </a>Alternatively, consider using CSS for the blurred background effect instead of loading the same image twice:
- template: `
- <a
- class="relative flex h-full items-center justify-center"
- [href]="cardBanner().navigateTo!"
- >
- <aside class="absolute h-full w-full overflow-hidden rounded-lg">
- <img
- tabindex="0"
- role="button"
- class="!relative object-cover shadow-inner blur-xl"
- [attr.aria-label]="cardBanner()!.alt!"
- [alt]="cardBanner().alt!"
- [ngSrc]="cardBanner().url!"
- fill
- priority
- />
- </aside>
- <aside>
- <img
- tabindex="0"
- role="button"
- class="!relative"
- [attr.aria-label]="cardBanner().alt!"
- [alt]="cardBanner().alt!"
- [ngSrc]="cardBanner().url!"
- fill
- priority
- />
- </aside>
- </a>
+ template: `
+ <a
+ class="relative flex h-full items-center justify-center"
+ [href]="cardBanner().navigateTo"
+ >
+ <div *ngIf="cardBanner().url" class="banner-container">
+ <img
+ class="banner-image"
+ [attr.aria-label]="cardBanner().alt || 'Newsletter banner'"
+ [alt]="cardBanner().alt || 'Newsletter banner'"
+ [ngSrc]="cardBanner().url"
+ fill
+ priority
+ />
+ </div>
+ </a>
+ `,
+ styles: [`
+ .banner-container {
+ position: relative;
+ width: 100%;
+ height: 100%;
+ overflow: hidden;
+ border-radius: 0.5rem;
+ }
+ .banner-container::before {
+ content: "";
+ position: absolute;
+ top: 0;
+ left: 0;
+ width: 100%;
+ height: 100%;
+ background-image: inherit;
+ background-size: cover;
+ filter: blur(16px);
+ z-index: 0;
+ }
+ .banner-image {
+ position: relative;
+ z-index: 1;
+ }
+ `]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template: ` | |
| <a | |
| class="relative flex h-full items-center justify-center" | |
| [href]="cardBanner().navigateTo!" | |
| > | |
| <aside class="absolute h-full w-full overflow-hidden rounded-lg"> | |
| <img | |
| tabindex="0" | |
| role="button" | |
| class="!relative object-cover shadow-inner blur-xl" | |
| [attr.aria-label]="cardBanner()!.alt!" | |
| [alt]="cardBanner().alt!" | |
| [ngSrc]="cardBanner().url!" | |
| fill | |
| priority | |
| /> | |
| </aside> | |
| <aside> | |
| <img | |
| tabindex="0" | |
| role="button" | |
| class="!relative" | |
| [attr.aria-label]="cardBanner().alt!" | |
| [alt]="cardBanner().alt!" | |
| [ngSrc]="cardBanner().url!" | |
| fill | |
| priority | |
| /> | |
| </aside> | |
| </a> | |
| `, | |
| template: ` | |
| <a | |
| class="relative flex h-full items-center justify-center" | |
| [href]="cardBanner().navigateTo" | |
| > | |
| <aside class="absolute h-full w-full overflow-hidden rounded-lg"> | |
| <img | |
| *ngIf="cardBanner().url" | |
| class="!relative object-cover shadow-inner blur-xl" | |
| [attr.aria-label]="cardBanner().alt || 'Newsletter banner'" | |
| [alt]="cardBanner().alt || 'Newsletter banner'" | |
| [ngSrc]="cardBanner().url" | |
| fill | |
| priority | |
| /> | |
| </aside> | |
| <aside> | |
| <img | |
| *ngIf="cardBanner().url" | |
| class="!relative" | |
| [attr.aria-label]="cardBanner().alt || 'Newsletter banner'" | |
| [alt]="cardBanner().alt || 'Newsletter banner'" | |
| [ngSrc]="cardBanner().url" | |
| fill | |
| priority | |
| /> | |
| </aside> | |
| </a> | |
| `, |
| // todo: remove this once the card banner is implemented | ||
| protected readonly cardBanner = computed(() => { | ||
| this._bannerStore.banners(); | ||
| return { | ||
| url: 'https://img.freepik.com/free-psd/banner-template-black-friday-clearance_23-2148745448.jpg', | ||
| alt: 'Card Banner Example', | ||
| navigateTo: '/', | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Temporary banner implementation needs improvement
The current implementation calls this._bannerStore.banners() but doesn't use the result, returning a hardcoded object instead.
Consider one of these alternatives:
- // todo: remove this once the card banner is implemented
- protected readonly cardBanner = computed(() => {
- this._bannerStore.banners();
- return {
- url: 'https://img.freepik.com/free-psd/banner-template-black-friday-clearance_23-2148745448.jpg',
- alt: 'Card Banner Example',
- navigateTo: '/',
- };
- });
+ // todo: remove this hardcoded fallback once the card banner is fully implemented
+ protected readonly cardBanner = computed(() => {
+ const banners = this._bannerStore.banners();
+ return banners?.cardBanner || {
+ url: 'https://img.freepik.com/free-psd/banner-template-black-friday-clearance_23-2148745448.jpg',
+ alt: 'Card Banner Example',
+ navigateTo: '/',
+ };
+ });Or if you want to keep it truly hardcoded for now:
- // todo: remove this once the card banner is implemented
- protected readonly cardBanner = computed(() => {
- this._bannerStore.banners();
- return {
- url: 'https://img.freepik.com/free-psd/banner-template-black-friday-clearance_23-2148745448.jpg',
- alt: 'Card Banner Example',
- navigateTo: '/',
- };
- });
+ // todo: replace with real banner implementation
+ protected readonly cardBanner = computed(() => ({
+ url: 'https://img.freepik.com/free-psd/banner-template-black-friday-clearance_23-2148745448.jpg',
+ alt: 'Card Banner Example',
+ navigateTo: '/',
+ }));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // todo: remove this once the card banner is implemented | |
| protected readonly cardBanner = computed(() => { | |
| this._bannerStore.banners(); | |
| return { | |
| url: 'https://img.freepik.com/free-psd/banner-template-black-friday-clearance_23-2148745448.jpg', | |
| alt: 'Card Banner Example', | |
| navigateTo: '/', | |
| }; | |
| }); | |
| // todo: remove this hardcoded fallback once the card banner is fully implemented | |
| protected readonly cardBanner = computed(() => { | |
| const banners = this._bannerStore.banners(); | |
| return banners?.cardBanner || { | |
| url: 'https://img.freepik.com/free-psd/banner-template-black-friday-clearance_23-2148745448.jpg', | |
| alt: 'Card Banner Example', | |
| navigateTo: '/', | |
| }; | |
| }); |
| is_card_banner_displayed: boolean; | ||
| card_banner_image: number /* mediaId */; | ||
| card_banner_url: string /* url to navigate to after click */; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Card banner missing mobile image support
Unlike slider and top banner, the card banner only has a single image property without device-specific versions.
Is this intentional, or should card banners also support device-specific images like the other banner types?
🏁 Script executed:
#!/bin/bash
# Check if other components use device-specific images for card banners
rg -A 3 -B 3 "card_banner_image" --type tsLength of output: 1306
I’d like to inspect the banner DTO definitions to confirm which types include device-specific images. Let me pull out the related sections for slider, mobile, and desktop in dtos.ts:
🏁 Script executed:
#!/bin/bash
# Show slider banner fields
rg -n "slider" -A5 -B5 libs/blog-bff/banners/api/src/lib/dtos.ts
# Check for mobile-specific image fields
rg -n "mobile" -A5 -B5 libs/blog-bff/banners/api/src/lib/dtos.ts
# Check for desktop-specific image fields
rg -n "desktop" -A5 -B5 libs/blog-bff/banners/api/src/lib/dtos.tsLength of output: 1786
Ensure Device-Specific Images for Card Banners
The card_banner DTO currently only exposes a single card_banner_image, whereas both the slider and top banners define separate desktop and mobile image fields. To maintain consistency and support mobile layouts, please split the card banner image into desktop and mobile variants (or confirm and document that a single image is intentional).
• In libs/blog-bff/banners/api/src/lib/dtos.ts
Replace:
card_banner_image: number /* mediaId */;With:
- card_banner_image: number /* mediaId */;
+ card_banner_image_desktop: number /* mediaId */;
+ card_banner_image_mobile: number /* mediaId */;• In libs/blog-bff/banners/api/src/lib/mappers.ts
Update the cardBanner mapping to source both URLs:
cardBanner: {
- url: mediaDto.find((m) => m.id === dto.acf.card_banner_image)?.guid.rendered,
+ desktopUrl: mediaDto.find((m) => m.id === dto.acf.card_banner_image_desktop)
+ ?.guid.rendered,
+ mobileUrl: mediaDto.find((m) => m.id === dto.acf.card_banner_image_mobile)
+ ?.guid.rendered,
navigateTo: dto.acf.card_banner_url,
},If the single-image design is intentional, please add a comment in the DTO or README explaining why card banners differ.
| ...(dto.acf.is_top_banner_displayed && { | ||
| topBanner: { | ||
| url: mediaDto.find( | ||
| (media) => media.id === dto.acf.top_banner_image_desktop, | ||
| )?.guid.rendered, | ||
| alt: mediaDto.find( | ||
| (media) => media.id === dto.acf.top_banner_image_desktop, | ||
| )?.alt_text, | ||
| navigateTo: dto.acf.top_banner_image_url, | ||
| }, | ||
| }), | ||
| ...(dto.acf.is_card_banner_displayed && { | ||
| cardBanner: { | ||
| url: mediaDto.find((media) => media.id === dto.acf.card_banner_image) | ||
| ?.guid.rendered, | ||
| alt: mediaDto.find((media) => media.id === dto.acf.card_banner_image) | ||
| ?.alt_text, | ||
| navigateTo: dto.acf.card_banner_url, | ||
| }, | ||
| }), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Code duplication in media lookups
There's significant duplication in the media lookup logic across all banner types, and mobile images aren't being used.
Consider extracting the media lookup logic into a helper function and using mobile images for responsive design:
export const toBanner = (
dto: WPBannerDto,
mediaDto: WPBannerMediaDto[],
): Banners => {
+ const findMedia = (mediaId: number) => mediaDto.find(media => media.id === mediaId);
+
return {
...(dto.acf.is_slider_banner_displayed && {
slider: {
slideDisplayTimeMs: +dto.acf.display_time,
slides: dto.acf.slides.map((slide) => {
- const media = mediaDto.find(
- (media) => media.id === slide.slide_image_desktop,
- )!;
+ const desktopMedia = findMedia(slide.slide_image_desktop);
+ const mobileMedia = findMedia(slide.slide_image_mobile);
return {
- url: media?.guid.rendered,
- alt: media?.alt_text,
+ url: desktopMedia?.guid.rendered,
+ mobileUrl: mobileMedia?.guid.rendered,
+ alt: desktopMedia?.alt_text,
navigateTo: slide.slide_url,
};
}),
},
}),
...(dto.acf.is_top_banner_displayed && {
topBanner: {
- url: mediaDto.find(
- (media) => media.id === dto.acf.top_banner_image_desktop,
- )?.guid.rendered,
- alt: mediaDto.find(
- (media) => media.id === dto.acf.top_banner_image_desktop,
- )?.alt_text,
+ url: findMedia(dto.acf.top_banner_image_desktop)?.guid.rendered,
+ mobileUrl: findMedia(dto.acf.top_banner_image_mobile)?.guid.rendered,
+ alt: findMedia(dto.acf.top_banner_image_desktop)?.alt_text,
navigateTo: dto.acf.top_banner_image_url,
},
}),
...(dto.acf.is_card_banner_displayed && {
cardBanner: {
- url: mediaDto.find((media) => media.id === dto.acf.card_banner_image)
- ?.guid.rendered,
- alt: mediaDto.find((media) => media.id === dto.acf.card_banner_image)
- ?.alt_text,
+ url: findMedia(dto.acf.card_banner_image)?.guid.rendered,
+ alt: findMedia(dto.acf.card_banner_image)?.alt_text,
navigateTo: dto.acf.card_banner_url,
},
}),
};
};Note: You would need to update the banner interfaces to include mobileUrl properties for this change.
Summary by CodeRabbit
New Features
Enhancements
Refactor
Style