fix: reference swipe container in carousel template #241
Conversation
Summary of ChangesHello @PieterjanDeClippel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the carousel component where template variables for slide height and index state were not correctly bound to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for vertical orientation in the carousel component and fixes an issue with template variable scope. The changes are well-structured, particularly the refactoring in BsSwipeContainerDirective to handle both horizontal and vertical orientations. I've identified a bug in the fade animation mode where the container height isn't correctly handled for vertical orientation. I've also suggested a couple of improvements for code clarity and to better align with Angular best practices, such as using the @let block in the template and removing a potentially redundant change detection call.
| <div class="carousel-indicators" [class.carousel-indicators-vertical]="orientation === 'vertical'" | ||
| [style.height.px]="orientation === 'vertical' ? (swipeContainer?.currentSlideHeight$ | async) : null"> |
There was a problem hiding this comment.
In fade animation mode, the swipeContainer is not available, as it's only defined within the slide animation block. Consequently, swipeContainer?.currentSlideHeight$ | async will always evaluate to null. This means that for a vertically oriented carousel with fade animation, the height of the indicators container will not be set, which could lead to layout issues. You should consider a different way to determine the height in fade mode, perhaps by creating a mechanism to measure slide heights that is independent of the swipe container.
| <div class="carousel slide mx-auto" [class.carousel-vertical]="orientation === 'vertical'" | ||
| [style.height.px]="orientation === 'vertical' ? (swipeContainer?.currentSlideHeight$ | async) : null"> | ||
| @if (indicators) { | ||
| <div class="carousel-indicators"> | ||
| <div class="carousel-indicators" [class.carousel-indicators-vertical]="orientation === 'vertical'" | ||
| [style.height.px]="orientation === 'vertical' ? (swipeContainer?.currentSlideHeight$ | async) : null"> | ||
| @for (image of (images$ | async); track image.id; let i = $index) { | ||
| <button type="button" (click)="container.goto(i)" | ||
| [class.active]="(container.imageIndex$ | async) === i" data-bs-target | ||
| [attr.aria-current]="(container.imageIndex$ | async) === i ? true : null" | ||
| <button type="button" (click)="swipeContainer?.goto(i)" | ||
| [class.active]="(swipeContainer?.imageIndex$ | async) === i" data-bs-target | ||
| [attr.aria-current]="(swipeContainer?.imageIndex$ | async) === i ? true : null" | ||
| [attr.aria-label]="'Slide ' + i"></button> | ||
| } | ||
| </div> |
There was a problem hiding this comment.
To improve readability and performance by reducing the number of async pipe subscriptions, you can use the @let syntax to create template variables for the resolved values of your observables. This also makes the template cleaner.
@let slideHeight = swipeContainer?.currentSlideHeight$ | async; @let imageIndex = swipeContainer?.imageIndex$ | async; <div class="carousel slide mx-auto" [class.carousel-vertical]="orientation === 'vertical'" [style.height.px]="orientation === 'vertical' ? slideHeight : null"> @if (indicators) { <div class="carousel-indicators" [class.carousel-indicators-vertical]="orientation === 'vertical'" [style.height.px]="orientation === 'vertical' ? slideHeight : null"> @for (image of (images$ | async); track image.id; let i = $index) { <button type="button" (click)="swipeContainer?.goto(i)" [class.active]="imageIndex === i" data-bs-target [attr.aria-current]="imageIndex === i ? true : null" [attr.aria-label]="'Slide ' + i"></button> } </div> | @Input() public set orientation(value: 'horizontal' | 'vertical') { | ||
| this._orientation = value ?? 'horizontal'; | ||
| this.cdRef.detectChanges(); | ||
| } |
There was a problem hiding this comment.
The explicit call to this.cdRef.detectChanges() within the setter for the orientation input is likely unnecessary. Angular's change detection mechanism automatically handles updates to the view when an @Input() property changes. Removing this call can simplify the code and rely on the framework's standard behavior. You should verify that the component continues to function as expected after its removal.
@Input() public set orientation(value: 'horizontal' | 'vertical') { this._orientation = value ?? 'horizontal'; }
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_690113aa332c8323a48356c8bcdd2689