feat: add static top banner for new ebook edition promotion #405
Conversation
WalkthroughThe pull request introduces a new Angular component, Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant R as RootShellComponent
participant T as TopBannerComponent
U->>R: Request page load
R->>T: Render TopBannerComponent
T-->>R: Return banner view
R-->>U: Render complete layout with banner and adjusted margins
Possibly related PRs
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ 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:
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 (
|
Deploying angular-love-client with
|
| Latest commit: |
07632cd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7c1732bb.angular-love-client.pages.dev |
| Branch Preview URL: | https://feat-static-top-banner.angular-love-client.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libs/blog/ad-banner/ui/src/lib/top-banner.component.ts (1)
3-19: Component structure looks good, but consider accessibility and user experience improvements.The component is well-structured with appropriate styling for a promotional banner. However, there are a few considerations:
- Missing accessibility attributes could make this banner difficult to navigate for screen reader users.
- There's no mechanism for users to dismiss the banner after seeing it.
- Hardcoded styles may make theming difficult in the future.
Consider these improvements:
@Component({ selector: 'al-top-banner', template: ` <a class="flex w-full flex-wrap justify-center gap-[5px] px-[10px] py-3 text-[18px] font-medium text-white" style="background-image: linear-gradient(0.25turn, #fe5758, #8d52fe)" href="https://houseofangular.io/the-ultimate-guide-to-angular-evolution/?utm_source=www-al&utm_medium=baner&utm_campaign=angular19-evolution" + aria-label="Promotional banner for Angular 19 ebook" + role="banner" > <span>Enhance Your Project with</span> <span class="rounded-[4px] bg-white px-[6px] text-[#f9506a]"> Angular 19 </span> <span class="underline">Download a free ebook!</span> </a> + <!-- Consider adding a close button here --> `, }) export class TopBannerComponent {}Also consider adding a method to allow users to dismiss the banner and store this preference:
export class TopBannerComponent { visible = true; dismissBanner(): void { this.visible = false; localStorage.setItem('bannerDismissed', 'true'); } ngOnInit(): void { this.visible = localStorage.getItem('bannerDismissed') !== 'true'; } }libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts (1)
25-25: Unused template reference variable.The
#topBannertemplate reference variable is defined but not used elsewhere in the template or component logic.Remove the unused reference variable unless you plan to use it:
-<al-top-banner #topBanner /> +<al-top-banner />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/blog/ad-banner/ui/src/index.ts(1 hunks)libs/blog/ad-banner/ui/src/lib/top-banner.component.ts(1 hunks)libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts(3 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
libs/blog/ad-banner/ui/src/lib/top-banner.component.ts (1)
libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts (1) (1)
Component(22:106)
libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts (2)
libs/blog/ad-banner/ui/src/lib/top-banner.component.ts (1) (1)
Component(3:19)libs/blog/ad-banner/ui/src/lib/banner-carousel/al-banner-carousel.component.ts (1) (1)
Component(7:36)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
libs/blog/ad-banner/ui/src/index.ts (1)
4-4: LGTM: Export of the new TopBannerComponent.The export follows the existing pattern and correctly exposes the new component through the module's public API.
libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts (2)
19-19: Import and component registration look good.The TopBannerComponent is correctly imported and added to the component imports array.
Also applies to: 55-55
97-103:❓ Verification inconclusive
Review viewport offset calculations.
The effect adjusts viewport offset based on
adBannerVisible(), but since this computed value is hardcoded to returnfalse, it will always use the [0, 80] offset. If the banner visibility should affect scrolling behavior, ensure this logic is consistent with the banner's actual visibility.
🏁 Script executed:
#!/bin/bash # Check for other usages of adBannerVisible that might be affected rg "adBannerVisible" --type tsLength of output: 1037
Viewport Offset Logic Verification
We confirmed that in libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts the computed property for
adBannerVisibleis hardcoded to returnfalse. This forces the offset to always be set to[0, 80], meaning the[0, 160]branch is never executed. The similar usage seen in the table of contents directive reinforces that the banner state is currently static. If the banner is intended to eventually display dynamically (and thus affect scrolling behavior), the computed value should be updated to reflect its actual visibility state.
- Location:
libs/blog/shell/feature-shell-web/src/lib/root-shell.component.ts(lines 97-103)- Observation:
protected readonly adBannerVisible = computed(() => false);ensures the offset is always[0, 80].- Recommendation: Update the computed logic if dynamic banner visibility is expected, or remove the condition if the banner state remains static.
| @@ -30,7 +32,7 @@ import { | |||
| <al-search /> | |||
| </al-header> | |||
| </div> | |||
| <al-layout class="mt-20" [ngClass]="{ 'mt-40': adBannerVisible() }"> | |||
| <al-layout class="mt-0" [ngClass]="{ 'mt-20': adBannerVisible() }"> | |||
There was a problem hiding this comment.
Banner visibility logic needs attention.
The adBannerVisible() computed value is hardcoded to return false, which means the conditional margin class mt-20 will never be applied. If the top banner should affect the layout, this needs to be updated.
Update the adBannerVisible computed property to reflect the actual visibility of the banner:
// todo: temporary solution to keep in mind how banner influence the layout
-protected readonly adBannerVisible = computed(() => false);
+protected readonly adBannerVisible = computed(() => true); // or implement actual logicAlternatively, if the banner should always be visible, consider removing the conditional and directly applying the desired margin:
-<al-layout class="mt-0" [ngClass]="{ 'mt-20': adBannerVisible() }">
+<al-layout class="mt-20">Also applies to: 76-77
Summary by CodeRabbit