Skip to content

feat(main): newsletter & top banners #380

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat(main): newsletter & top banners #380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5a7f82f
feat(main): newsletter & top banners
majahendzel-va Feb 6, 2025
87d4b61
feat: move newsletter banner to separate component, use navigateTo fo…
dokolyski Feb 12, 2025
db7cc3d
feat: merged main and made minor styling fixes
marcin-hoa May 14, 2025
fd83adb
feat: changed TopBanner type
marcin-hoa May 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/blog/src/assets/icons/arrow-down.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
21 changes: 15 additions & 6 deletions libs/blog-bff/banners/api/src/lib/dtos.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,22 @@
export interface WPBannerDto {
id: number;
acf: {
is_slider_banner_displayed: boolean;
display_time: string;
slides:
| {
slide_image: number /* slideId */;
slide_url: string /* url to navigate to after click */;
}[]
| null;
slides: {
slide_image_desktop: number /* slideId */;
slide_image_mobile: number /* slideId */;
slide_url: string /* url to navigate to after click */;
}[];

is_top_banner_displayed: boolean;
top_banner_image_desktop: number /* mediaId */;
top_banner_image_mobile: number /* mediaId */;
top_banner_image_url: string /* url to navigate to after click */;

is_card_banner_displayed: boolean;
card_banner_image: number /* mediaId */;
card_banner_url: string /* url to navigate to after click */;
Comment on lines +17 to +19
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 ts

Length 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.ts

Length 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.

};
}

Expand Down
49 changes: 37 additions & 12 deletions libs/blog-bff/banners/api/src/lib/mappers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,46 @@
import { Slider } from '@angular-love/blog/contracts/banners';
import { Banners } from '@angular-love/blog/contracts/banners';

import { WPBannerDto, WPBannerMediaDto } from './dtos';

export const toBanner = (
dto: WPBannerDto,
mediaDto: WPBannerMediaDto[],
): Slider => {
): Banners => {
return {
slideDisplayTimeMs: +dto.acf.display_time,
slides:
dto.acf.slides?.map((slide) => {
const media = mediaDto.find((media) => media.id === slide.slide_image)!;
return {
url: media.guid.rendered,
alt: media.alt_text,
navigateTo: slide.slide_url,
};
}) ?? [],
...(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,
)!;
return {
url: media?.guid.rendered,
alt: media?.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,
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,
},
}),
};
Comment on lines +25 to 45
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

};
23 changes: 21 additions & 2 deletions libs/blog-contracts/banners/src/lib/banners.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,27 @@
export interface Slider {
slideDisplayTimeMs: number;
slides: {
url: string;
alt: string;
url?: string;
alt?: string;
navigateTo: string;
}[];
}

export interface TopBanner {
navigateTo?: string;
buttonText?: string;
text: string;
buttonPosition: 'left' | 'right';
}

export interface CardBanner {
url?: string;
alt?: string;
navigateTo: string;
}

export interface Banners {
slider?: Slider;
topBanner?: TopBanner;
cardBanner?: CardBanner;
}
11 changes: 5 additions & 6 deletions libs/blog/ad-banner/data-access/src/lib/infrastructure/ad-banner.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
import { HttpClient } from '@angular/common/http';
import { inject, Injectable } from '@angular/core';
import { map } from 'rxjs';

import { Slider } from '@angular-love/blog/contracts/banners';
import { Banners } from '@angular-love/blog/contracts/banners';
import { ConfigService } from '@angular-love/shared/config';

@Injectable({ providedIn: 'root' })
export class AdBannerService {
private readonly _apiBaseUrl = inject(ConfigService).get('apiBaseUrl');
private readonly _http = inject(HttpClient);

getBannerSlider() {
return this._http
.get<{ banners: Slider }>(`./assets/banners.json`)
.pipe(map((resp) => resp.banners));
getVisibleBanners() {
return this._http.get<Banners>(`${this._apiBaseUrl}/banners`);
Comment on lines +12 to +13
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. State Transfer with TransferState API
    Angular provides a TransferState API 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 TransferState first. If data is available, skip the HTTP call.
      This efficiently reduces redundant calls since the client can reuse server-fetched data.

  2. Caching HTTP Requests
    Use an HTTP caching mechanism on the server during SSR. You can implement operators on the HTTP service like shareReplay to 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 TransferState for best results.

  3. 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.

  4. 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.

  5. 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.

  6. 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.

  7. Universal-aware Data Services
    Design services that can detect if they are running on the server or client (using isPlatformServer) to conditionally trigger API calls or fetch from caches.

  8. 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

}
Comment on lines +12 to 14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, we can do that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this pr resolves the issue #415

}
12 changes: 6 additions & 6 deletions libs/blog/ad-banner/data-access/src/lib/state/ad-banner.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,16 @@ import { patchState, signalStore, withMethods, withState } from '@ngrx/signals';
import { rxMethod } from '@ngrx/signals/rxjs-interop';
import { pipe, switchMap } from 'rxjs';

import { Slider } from '@angular-love/blog/contracts/banners';
import { Banners } from '@angular-love/blog/contracts/banners';

import { AdBannerService } from '../infrastructure/ad-banner.service';

type AdBannerState = {
slider: Slider | null;
banners: Banners | null;
};

const initialState: AdBannerState = {
slider: null,
banners: null,
};

export const AdBannerStore = signalStore(
Expand All @@ -23,10 +23,10 @@ export const AdBannerStore = signalStore(
getData: rxMethod<void>(
pipe(
switchMap(() =>
adBannerService.getBannerSlider().pipe(
adBannerService.getVisibleBanners().pipe(
tapResponse({
next: (slider) => {
patchState(store, { slider });
next: (banners) => {
patchState(store, { banners });
},
error: () => {
patchState(store);
Expand Down
3 changes: 2 additions & 1 deletion libs/blog/ad-banner/ui/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export * from './lib/ad-image-banner/ad-image-banner.component';
export * from './lib/ad-image-banner/ad-image-banner-data.interface';
export * from './lib/ad-image-banner/ad-image-banner.component';
export * from './lib/banner-carousel/al-banner-carousel.component';
export * from './lib/newsletter-banner/al-newsletter-banner.component';
export * from './lib/top-banner.component';
44 changes: 44 additions & 0 deletions libs/blog/ad-banner/ui/src/lib/newsletter-banner/al-newsletter-banner.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { NgOptimizedImage } from '@angular/common';
import { ChangeDetectionStrategy, Component, input } from '@angular/core';

import { CardBanner } from '@angular-love/blog/contracts/banners';

@Component({
selector: 'al-newsletter-banner',
imports: [NgOptimizedImage],
changeDetection: ChangeDetectionStrategy.OnPush,
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>
`,
Comment on lines +10 to +40
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve template safety and accessibility.

The template has several issues that should be addressed:

  1. Non-null assertions (!) are used extensively, which could lead to runtime errors if properties are undefined.
  2. Duplicate images are loaded for the same content, which is inefficient.
  3. 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.

Suggested change
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>
`,

})
export class AlNewsletterBannerComponent {
readonly cardBanner = input.required<CardBanner>();
}
4 changes: 2 additions & 2 deletions libs/blog/articles/feature-article/src/lib/article-details/article-details.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ <h2 id="article-title" class="flex text-[40px] font-bold">
<div
class="sticky top-24 mt-5 hidden flex-col gap-4 lg:flex"
[ngClass]="{
'top-24': !adBannerStoreVisible(),
'top-48': adBannerStoreVisible(),
'top-24': !bannerStore.banners()?.topBanner,
'top-48': bannerStore.banners()?.topBanner,
}"
>
@if (articleDetails().anchors.length) {
Expand Down
5 changes: 3 additions & 2 deletions libs/blog/articles/feature-article/src/lib/article-details/article-details.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@ import { DatePipe, NgClass } from '@angular/common';
import {
ChangeDetectionStrategy,
Component,
inject,
input,
signal,
} from '@angular/core';
import { FastSvgComponent } from '@push-based/ngx-fast-svg';

import { AdBannerStore } from '@angular-love/blog/ad-banner/data-access';
import { GiscusCommentsComponent } from '@angular-love/blog/articles/feature-comments';
import { RelatedArticlesComponent } from '@angular-love/blog/articles/feature-related-articles';
import { ArticleCompactCardSkeletonComponent } from '@angular-love/blog/articles/ui-article-card';
Expand Down Expand Up @@ -57,5 +58,5 @@ import { ArticleShareIconsComponent } from '../article-share-icons/article-share
})
export class ArticleDetailsComponent {
readonly articleDetails = input.required<Article>();
protected readonly adBannerStoreVisible = signal(false);
protected readonly bannerStore = inject(AdBannerStore);
}
21 changes: 15 additions & 6 deletions ...re-latest-articles/src/lib/feature-latest-articles/feature-latest-articles.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,21 @@
} @else {
<al-article-regular-card-skeleton *alRepeat="take" />
}
<al-card
alGradientCard
class="md:max-lg:col-span-2 lg:col-start-3 lg:row-start-1"
>
<al-newsletter alCardContent />
</al-card>

@if (cardBanner(); as cardBanner) {
<al-newsletter-banner
alCardContent
class="md:max-lg:col-span-2 lg:col-start-3 lg:row-start-1"
[cardBanner]="cardBanner"
/>
} @else {
<al-card
alGradientCard
class="md:max-lg:col-span-2 lg:col-start-3 lg:row-start-1"
>
<al-newsletter alCardContent />
</al-card>
}
</section>

<div class="flex justify-center gap-3">
Expand Down
22 changes: 16 additions & 6 deletions ...ture-latest-articles/src/lib/feature-latest-articles/feature-latest-articles.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,18 @@ import {
import { RouterLink } from '@angular/router';
import { TranslocoDirective } from '@jsverse/transloco';

import { AdBannerStore } from '@angular-love/blog/ad-banner/data-access';
import { ArticleListStore } from '@angular-love/blog/articles/data-access';
import {
ArticleRegularCardSkeletonComponent,
UiArticleCardComponent,
} from '@angular-love/blog/articles/ui-article-card';
import { NewsletterComponent } from '@angular-love/blog/newsletter';
import { AlNewsletterBannerComponent } from '@angular-love/blog/shared/ad-banner';
import { ButtonComponent } from '@angular-love/blog/shared/ui-button';
import {
CardComponent,
CardContentDirective,
GradientCardDirective,
} from '@angular-love/blog/shared/ui-card';
import { PillDirective } from '@angular-love/blog/shared/ui-pill';
Expand All @@ -34,15 +37,16 @@ import { CategoryListItem, injectCategories } from './categories.const';
UiArticleCardComponent,
NewsletterComponent,
CardComponent,
GradientCardDirective,
NgClass,
TranslocoDirective,
ArticleRegularCardSkeletonComponent,
CardComponent,
RepeatDirective,
CardContentDirective,
AlNewsletterBannerComponent,
RouterLink,
ButtonComponent,
PillDirective,
GradientCardDirective,
],
host: {
'data-testid': 'latest-articles-container',
Expand All @@ -61,13 +65,19 @@ export class FeatureLatestArticlesComponent {
readonly take = 8;

private readonly _articleListStore = inject(ArticleListStore);
private readonly _bannerStore = inject(AdBannerStore);

// 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: '/',
};
});
Comment on lines +70 to +78
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
// 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: '/',
};
});

readonly isFetchArticleListLoading =
this._articleListStore.isFetchArticleListLoading;

readonly isFetchArticleListError =
this._articleListStore.isFetchArticleListError;

readonly articles = this._articleListStore.articles;

constructor() {
Expand Down
2 changes: 1 addition & 1 deletion ...le-card/src/lib/components/article-horizontal-card/article-horizontal-card.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<a [routerLink]="['/', article().slug] | alLocalize">
<article
class="group relative flex h-full w-full flex-row rounded-lg shadow-none max-h-52"
class="group relative flex h-full max-h-52 w-full flex-row rounded-lg shadow-none"
[attr.aria-labelledby]="article().slug"
>
<img
Expand Down
Loading