Replace Angular animations with browser native animation#600
Replace Angular animations with browser native animation#600uldisrudzitis merged 2 commits intomasterfrom
Conversation
uldisrudzitis
added
Enhancement
dependencies
uldisrudzitis
changed the title
WalkthroughThis PR removes Angular animation usage across the editor: deletes BrowserAnimationsModule and the shared animations file, strips animation bindings/imports from multiple components, restructures templates to non-animated layouts, and replaces an overflow-based expansion with a grid-based CSS transition. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant S as ShopComponent
participant R as ActivatedRoute
participant N as Router
U->>S: click toggleSection(section)
Note over S: async toggleSection\nawait currentShopSection$
S->>R: read route params (currentShopSection$)
R-->>S: returns section (via observable)
alt same section
S->>N: navigate to shop root (clear section)
else different section
S->>N: navigate to `/shop/<section>`
end
N-->>S: navigation result
S->>U: UI updates via route-driven bindings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (6)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
editor/src/app/sites/media/entry-gallery-image-editor.component.ts (1)
27-28: @Component.imports withoutstandalone: truecauses compilation error in Angular 20.The
importsproperty in @component decorator is only valid for standalone components. This component is declared inSiteMediaModule, making it non-standalone.Required fixes:
- Remove
importsfrom the decorator and setstandalone: false:@Component({ selector: 'berta-entry-gallery-image-editor', + standalone: false, - imports: [ImageCropperComponent, SitesSharedModule], template: `
- In
site-media.module.ts, move the component fromimportstodeclarationsand importImageCropperModule:@NgModule({ imports: [ CommonModule, RouterModule.forChild([]), NgsgModule, SitesSharedModule, + ImageCropperModule, - EntryGalleryImageEditorComponent, ], declarations: [ SiteMediaComponent, EntryGalleryComponent, EntryGalleryEditorComponent, + EntryGalleryImageEditorComponent, ], })Add the import at the top of
site-media.module.ts:import { ImageCropperModule } from 'ngx-image-cropper';
🧹 Nitpick comments (7)
editor/src/styles/_aside.scss (2)
175-182: Nice CSS grid expander; consider reduced‑motion.Grid 0fr→1fr transition is clean. Add an opt‑out for users with reduced motion preferences.
> .settings { display: grid; grid-template-rows: 0fr; transition: grid-template-rows 0.25s ease; > div { overflow: hidden; } } + @media (prefers-reduced-motion: reduce) { + > .settings { + transition: none; + } + }
278-279: Preserve border state during transition.When toggling, the bottom border appears only in expanded state. If you want a consistent divider regardless of motion setting, move the border to the container and toggle color/alpha instead. Optional, stylistic.
editor/src/app/shop/shop.component.ts (1)
40-66: Add keyboard support to the section toggle.
(click)on<h3>isn’t keyboard‑accessible. Provide Enter/Space handlers and tabindex.// In the <h3> element <h3 (click)="toggleSection(shopSection.urlSegment)" (keydown.enter)="toggleSection(shopSection.urlSegment)" (keydown.space)="toggleSection(shopSection.urlSegment)" [attr.tabindex]="0" role="button" class="hoverable" >editor/src/app/sites/sections/background-gallery-editor.component.ts (2)
52-67: Make the “Item settings” toggle accessible.Add Enter/Space handlers and a tabindex on the
<h3>that togglesfileSettingsIsOpen.<h3 (click)="fileSettingsIsOpen = !fileSettingsIsOpen" (keydown.enter)="fileSettingsIsOpen = !fileSettingsIsOpen" (keydown.space)="fileSettingsIsOpen = !fileSettingsIsOpen" [attr.tabindex]="0" role="button" class="hoverable" >
92-99: Mirror the same a11y for “Gallery settings”.Apply the same key handlers/tabindex to the gallery header.
editor/src/app/sites/media/entry-gallery-editor.component.ts (2)
54-106: Add keyboard support to open/close “Item settings”.Mirror the a11y tweak: Enter/Space handlers and tabindex on the toggling
<h3>.
133-344: Minor: preferforEach(or for..of) overmapfor side effects.
files.map(…)ignores the returned array. UseforEachorfor..offor clarity.- files.map((file) => { + files.forEach((file) => { this.store .dispatch(/* … */) .pipe(take(1)) .subscribe( () => {}, (error) => { this.uploadFilesErrors.push(error.error.error); }, ); - }); + });Also add the same Enter/Space handlers/tabindex on the “Gallery settings” header.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
editor/src/app/app.module.ts(0 hunks)editor/src/app/shared/animations.ts(0 hunks)editor/src/app/shop/shop.component.ts(1 hunks)editor/src/app/sites/media/entry-gallery-editor.component.ts(5 hunks)editor/src/app/sites/media/entry-gallery-image-editor.component.ts(1 hunks)editor/src/app/sites/sections/background-gallery-editor.component.ts(2 hunks)editor/src/app/sites/sections/section.component.ts(1 hunks)editor/src/app/sites/settings/site-settings.component.ts(1 hunks)editor/src/app/sites/template-settings/site-template-settings.component.ts(1 hunks)editor/src/styles/_aside.scss(2 hunks)
💤 Files with no reviewable changes (2)
- editor/src/app/shared/animations.ts
- editor/src/app/app.module.ts
🔇 Additional comments (2)
editor/src/app/sites/sections/section.component.ts (1)
78-133: LGTM – structure matches the new CSS expander semantics.editor/src/app/sites/settings/site-settings.component.ts (1)
61-148: Fix children reuse logic in scan (null guard + correct comparison).Two issues can throw or prevent proper referential reuse:
- Potential NPE:
prevSettingChildren.lengthwhenchildrenis undefined.- Likely typo: comparing
prevChild.setting === child.configinstead ofchild.setting.Apply:
- const prevSettingChildren = prevSettingGroup.settings[index].children; - if (prevSettingChildren.length > 0) { + const prevSettingChildren = prevSettingGroup.settings[index].children; + if (prevSettingChildren && prevSettingChildren.length > 0) { setting.children = setting.children.map((row, index) => { const prevSettingRow = prevSettingChildren[index]; if (prevSettingRow) { return row.map((child, i) => { const prevChild = prevSettingRow[i]; - if (prevChild && prevChild.setting === child.config) { + if (prevChild && prevChild.setting === child.setting) { return prevChild; } return child; }); } return row; }); }I can open a follow‑up to add unit tests for this reuse path.
Likely an incorrect or invalid review comment.
| <div class="setting"> | ||
| @if (cropperIsReady) { | ||
| <button | ||
| type="button" | ||
| class="button" | ||
| [class.disabled]="!canCrop" | ||
| (click)="cropImage()" | ||
| > | ||
| Crop | ||
| </button> | ||
| } | ||
| <button | ||
| type="button" | ||
| class="button" | ||
| [class.disabled]="!canCrop" | ||
| (click)="cropImage()" | ||
| class="button inverse" | ||
| (click)="navigateBack()" | ||
| > | ||
| Crop | ||
| Close | ||
| </button> | ||
| } | ||
| <button | ||
| type="button" | ||
| class="button inverse" | ||
| (click)="navigateBack()" | ||
| > | ||
| Close | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
Prevent invalid cropping actions (runtime safety + UX).
- Button uses a CSS “disabled” class only; clicks still fire.
- Methods assume
imageCroppedEventis set.
Add [disabled] and guards.
-<button
+<button
type="button"
class="button"
- [class.disabled]="!canCrop"
+ [class.disabled]="!canCrop"
+ [disabled]="!canCrop"
(click)="cropImage()"
>
Crop
</button>- cropImage() {
+ cropImage() {
+ if (!this.imageCroppedEvent) { return; }
this.store.dispatch(
new UpdateEntryGalleryImageCropAction(
this.site.name,
this.sectionName,
this.entryId,
this.imageOrder.toString(),
{
x: this.imageCroppedEvent.imagePosition.x1,
y: this.imageCroppedEvent.imagePosition.y1,
width: this.imageCroppedEvent.width,
height: this.imageCroppedEvent.height,
},
),
);
}Optionally guard updateSize(e) similarly:
if (!this.imageCroppedEvent) { return; }🤖 Prompt for AI Agents
In editor/src/app/sites/media/entry-gallery-image-editor.component.ts around
lines 88 to 106, the Crop button only uses a CSS class to look disabled but
still fires clicks and methods assume imageCroppedEvent exists; update the
template to add a real disabled attribute (e.g. [disabled]="!canCrop") so clicks
are prevented by the browser and then add runtime guards in the component
methods: at the start of cropImage() check if (!this.canCrop ||
!this.imageCroppedEvent) return; and similarly add if (!this.imageCroppedEvent)
return; to updateSize(e) to avoid null/undefined access. Ensure the Close button
behavior is unchanged.
| <div class="settings"> | ||
| <div> | ||
| @for (setting of settingGroup.settings; track setting) { | ||
| <berta-setting | ||
| [templateSlug]="settingGroup.templateSlug" | ||
| [setting]="setting.setting" | ||
| [config]="setting.config" | ||
| [disabled]=" | ||
| settingUpdate[settingGroup.slug + ':' + setting.setting.slug] | ||
| " | ||
| [error]=" | ||
| settingError[settingGroup.slug + ':' + setting.setting.slug] | ||
| " | ||
| (update)="updateSetting(settingGroup.slug, $event)" | ||
| (emitAction)="emitAction($event)" | ||
| ></berta-setting> | ||
| } | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix type mismatch and RxJS scan signature (build-time blocker).
The template uses settingGroup.templateSlug, but the declared type puts templateSlug on each item in settings[]. Mapping code also attaches templateSlug on the group, not on items. This inconsistency will fail strict template type‑checking and confuses tooling. Also, scan((prev, curr, templateSlug) => …) names the third param as if it were a slug; it’s actually the emission index.
- Align the type so
templateSluglives on the group. - Simplify
scanto the 2‑arg form (or rename the 3rd param to_indexand remove the falsy check).
Apply the following fixes:
Type shape:
// change the group shape so templateSlug is on the group, not items
templateSettings$: Observable<Array<{
config: SettingGroupConfigModel['_'];
settings: Array<{
setting: SettingModel;
config: SettingConfigModel;
}>;
slug: string;
templateSlug: string;
}>>;scan:
// current
scan((prevSettingGroups, settingGroups, templateSlug) => {
// proposed
scan((prevSettingGroups, settingGroups) => {
if (!prevSettingGroups || prevSettingGroups.length === 0) {
return settingGroups;
}
// …rest unchanged
})🤖 Prompt for AI Agents
In editor/src/app/sites/template-settings/site-template-settings.component.ts
around lines 58 to 75, the template expects templateSlug on the settingGroup but
the TypeScript shape places templateSlug on each setting item and the RxJS scan
callback uses a 3‑arg signature (third param misnamed templateSlug) which breaks
strict type checking and the build; update the group type so templateSlug is a
property of the settingGroup (remove it from individual setting items) and
adjust the scan operator to the 2‑argument form scan((prev, curr) => { ... })
(or rename the third param to _index and drop any falsy-checks that treated it
as a slug) so the types and template align and the RxJS callback uses the
correct parameters.
1 participant
Summary by CodeRabbit
Refactor
Style