#98: Form error message directive #100
Conversation
|
Warning Rate limit exceeded@thongdanghoang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (11)
WalkthroughThe pull request introduces significant changes aimed at enhancing theme configuration, form validation, and error handling in the frontend application. Key updates include the replacement of the Material theme with the Aura theme, the addition of new directives and components for improved error messaging, and a streamlined validation mechanism for the user creation form. These modifications collectively aim to standardize user interface elements, improve maintainability, and enhance the overall user experience across the application. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts (2)
22-23: Consider initializing TranslateParamsPipe in ngOnInit.The pipe instance is created in the constructor but not cleaned up. Consider moving initialization to ngOnInit and cleanup in ngOnDestroy to prevent potential memory leaks.
62-66: Consider using Angular's template-driven approach instead of direct DOM manipulation.Using ElementRef.nativeElement.innerText for error display bypasses Angular's change detection and sanitization. Consider using a template-driven approach with *ngIf or [innerHTML] with sanitization.
sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.ts (1)
117-132: Consider adding null safety checks and early returns.The validator implementation could be more robust with additional checks.
Consider this improved implementation:
private buildingValidator(): ValidatorFn { return (control: AbstractControl): ValidationErrors | null => { - if (!this.enterpriseUserStructure) { + if (!this.enterpriseUserStructure?.scope?.value) { return null; } - if ( - this.enterpriseUserStructure.scope.value === - UserScope[UserScope.BUILDING] - ) { - if (control.value?.length === 0) { - return {required: true}; - } + + const isBuildingScope = this.enterpriseUserStructure.scope.value === UserScope[UserScope.BUILDING]; + if (!isBuildingScope) { + return null; } - return null; + + return (!control.value || control.value.length === 0) ? {required: true} : null; }; }sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.html (1)
135-154: Consider adding loading state and error handling for building data.The building selection implementation could be improved for better user experience.
Consider these enhancements:
<div class="md:col-span-3 col-span-12" *ngIf="enterpriseUserStructure.scope.value === UserScope[UserScope.BUILDING]" errorMessages > <p-multiselect class="w-full" [options]="mockBuildings" optionLabel="name" optionValue="id" [placeholder]="'enterprise.Users.placeholders.selectBuildings' | translate" lazy="true" + [loading]="isLoadingBuildings" + [emptyMessage]="'enterprise.Users.noBuildings' | translate" + [showClear]="true" + [filter]="true" formControlName="buildings" ></p-multiselect> <form-field-error></form-field-error> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
sep490-frontend/src/app/modules/core/services/theme.service.ts(1 hunks)sep490-frontend/src/app/modules/dev/components/toolbox/toolbox.component.html(1 hunks)sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.html(2 hunks)sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.ts(3 hunks)sep490-frontend/src/app/modules/shared/components/form/form-field-error/form-field-error.component.html(1 hunks)sep490-frontend/src/app/modules/shared/components/form/form-field-error/form-field-error.component.ts(1 hunks)sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts(1 hunks)sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts(1 hunks)sep490-frontend/src/app/modules/shared/shared.module.ts(2 hunks)sep490-frontend/tailwind.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- sep490-frontend/src/app/modules/shared/components/form/form-field-error/form-field-error.component.html
- sep490-frontend/src/app/modules/shared/components/form/form-field-error/form-field-error.component.ts
🧰 Additional context used
🪛 Biome (1.9.4)
sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts
[error] 43-43: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (7)
sep490-frontend/tailwind.config.js (1)
7-23: LGTM! Well-structured color palette.The red color palette provides a comprehensive range of shades with proper contrast ratios, following Tailwind's color system best practices.
sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts (1)
86-89: Add null check for querySelector result.The querySelector might return null if no matching element is found. Consider adding appropriate error handling.
sep490-frontend/src/app/modules/shared/shared.module.ts (1)
78-81: LGTM! Well-structured module organization.The new directives and component are properly declared and exported, following Angular's module best practices.
Also applies to: 89-92
sep490-frontend/src/app/modules/core/services/theme.service.ts (1)
3-3: LGTM! Theme migration from Material to Aura.The switch to Aura theme aligns with modern design principles while maintaining the application's functionality.
Also applies to: 7-7
sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.ts (1)
56-59: LGTM! Clean form control definition with custom validator.The form control is well-structured with the custom validator bound to the component instance.
sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.html (1)
11-27: LGTM! Well-structured form field with error handling.The email field implementation follows best practices with proper error handling and accessibility attributes.
sep490-frontend/src/app/modules/dev/components/toolbox/toolbox.component.html (1)
39-48: LGTM! Comprehensive button showcase with consistent layout.The implementation provides a well-organized display of button variations with proper spacing and alignment. The use of flex containers ensures responsive layout.
Also applies to: 50-59, 61-70, 72-81, 83-91, 93-101, 103-111, 113-121, 123-131
sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts
Show resolved
Hide resolved
sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts
Show resolved
Hide resolved
sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts
Outdated
Show resolved
Hide resolved
thongdanghoang
force-pushed
the
feature/68-code-base-improvement
branch
from
fa76042 to
84760d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts (1)
62-66: Improve type safety in showErrors method.The method could benefit from more specific typing and null checks.
- private showErrors(errors: ValidationErrors): void { + private showErrors(errors: ValidationErrors | null): void { + if (!this.el?.nativeElement) return; this.el.nativeElement.innerText = Object.keys(errors || {}) .map(key => this.pipe.transform(`validation.${key}`, errors[key])) .join('\n'); }sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.ts (1)
117-132: Optimize buildingValidator implementation.The validator implementation can be simplified and made more robust.
private buildingValidator(): ValidatorFn { return (control: AbstractControl): ValidationErrors | null => { - if (!this.enterpriseUserStructure) { - return null; - } - if ( - this.enterpriseUserStructure.scope.value === - UserScope[UserScope.BUILDING] - ) { - if (control.value?.length === 0) { - return {required: true}; - } + const scopeValue = this.enterpriseUserStructure?.scope?.value; + if (scopeValue !== UserScope[UserScope.BUILDING]) { + return null; } - return null; + return (!control.value || control.value.length === 0) + ? {required: true} + : null; }; }sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.html (1)
Line range hint
11-154: Enhance form accessibility and maintainability.The form structure has several areas for improvement:
- Add ARIA labels for better screen reader support
- Consider extracting repeated form field patterns into a reusable component
- Add form-level error handling
Example improvement for a form field:
<div class="grid grid-cols-12 gap-4 items-center my-3" errorMessages> <label for="email" class="font-normal md:col-span-3 col-span-12">{{ "enterprise.Users.email" | translate }}</label> <div class="md:col-span-6 col-span-12"> <input formControlName="email" class="w-full" type="text" id="email" pInputText + [attr.aria-label]="'enterprise.Users.email' | translate" + [attr.aria-invalid]="formGroup.get('email')?.invalid" + [attr.aria-describedby]="'email-error'" [placeholder]=" 'enterprise.Users.placeholders.enterEmail' | translate " /> - <form-field-error></form-field-error> + <form-field-error id="email-error"></form-field-error> </div> </div>Consider creating a reusable form field component:
@Component({ selector: 'app-form-field', template: ` <div class="grid grid-cols-12 gap-4 items-center my-3" errorMessages> <label [for]="id" class="font-normal md:col-span-3 col-span-12"> {{ label | translate }} </label> <div class="md:col-span-6 col-span-12"> <ng-content></ng-content> <form-field-error [id]="id + '-error'"></form-field-error> </div> </div> ` }) export class FormFieldComponent { @Input() id!: string; @Input() label!: string; }sep490-frontend/src/app/modules/dev/components/toolbox/toolbox.component.html (1)
39-131: Optimize button showcase implementation.The current implementation has repetitive patterns and could be optimized for better maintainability and performance.
Consider using a more data-driven approach:
interface ButtonConfig { label?: string; icon?: string; severity?: string; variant?: string; raised?: boolean; rounded?: boolean; outlined?: boolean; text?: boolean; } interface ButtonGroup { title: string; buttons: ButtonConfig[]; } const buttonGroups: ButtonGroup[] = [ { title: 'Raised Buttons', buttons: [ { label: 'Primary', raised: true }, { label: 'Secondary', raised: true, severity: 'secondary' }, // ... other configurations ] }, // ... other groups ];Then in the template:
<div *ngFor="let group of buttonGroups" class="flex justify-center flex-wrap gap-4 mb-6"> <p-button *ngFor="let btn of group.buttons" [label]="btn.label" [icon]="btn.icon" [severity]="btn.severity" [variant]="btn.variant" [raised]="btn.raised" [rounded]="btn.rounded" [outlined]="btn.outlined" [text]="btn.text"> </p-button> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
sep490-frontend/src/app/modules/core/services/theme.service.ts(1 hunks)sep490-frontend/src/app/modules/dev/components/toolbox/toolbox.component.html(1 hunks)sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.html(2 hunks)sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.ts(3 hunks)sep490-frontend/src/app/modules/shared/components/form/form-field-error/form-field-error.component.html(1 hunks)sep490-frontend/src/app/modules/shared/components/form/form-field-error/form-field-error.component.ts(1 hunks)sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts(1 hunks)sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts(1 hunks)sep490-frontend/src/app/modules/shared/shared.module.ts(2 hunks)sep490-frontend/tailwind.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- sep490-frontend/src/app/modules/shared/components/form/form-field-error/form-field-error.component.html
- sep490-frontend/src/app/modules/shared/components/form/form-field-error/form-field-error.component.ts
- sep490-frontend/tailwind.config.js
- sep490-frontend/src/app/modules/shared/shared.module.ts
- sep490-frontend/src/app/modules/core/services/theme.service.ts
🧰 Additional context used
🪛 Biome (1.9.4)
sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts
[error] 43-43: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (2)
sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts (1)
43-43: Avoid assignment in tap operator.The assignment within the tap operator makes the code harder to understand and maintain. Consider using a separate variable.
- tap(errors => (this.errors = errors)), + tap(errors => { + this.errors = errors; + }),🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts (1)
60-66: Improve type safety in event methods.Methods returning 'any' can lead to runtime errors. Consider adding proper return types.
- formSubmitEvent(): any { + formSubmitEvent(): Observable<Event> { return this.form.ngSubmit; } - controlStatusChanges(): any { + controlStatusChanges(): Observable<any> | undefined { return this.ngControl?.statusChanges; }
sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts
Show resolved
Hide resolved
sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts
Show resolved
Hide resolved
thongdanghoang
force-pushed
the
feature/68-code-base-improvement
branch
from
84760d4 to
bb4c7ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
sep490-frontend/src/app/modules/core/services/theme.service.ts (1)
8-22:⚠️ Potential issue
⚠️ Accessibility Issues Found: Color Contrast Ratios Need AdjustmentThe red color palette, particularly shades 500 (#ef797a) and 600 (#cb6768), fails to meet WCAG AA accessibility requirements for normal text. For error messages and critical UI elements, consider using red-700 (#a75555) or adjusting the colors to achieve higher contrast ratios.
Consider adjusting the red palette to meet accessibility standards:
primitive: { red: { - 500: '#ef797a', - 600: '#cb6768', + 500: '#d65657', // Adjusted for better contrast + 600: '#b94445', // Adjusted for better contrast } }
🧹 Nitpick comments (3)
sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts (2)
31-36: Optimize language change subscription.The language change subscription could be combined with the error display logic to avoid unnecessary renders.
- translate.onLangChange - .pipe(takeUntil(this.destroy$)) - .subscribe((): void => this.showErrors(this.errors || {})); + if (this.control) { + merge( + translate.onLangChange, + this.control.errors$ + ).pipe( + takeUntil(this.destroy$) + ).subscribe(() => { + this.showErrors(this.errors || {}); + }); + }
47-58: Extract complex condition to a method.The condition checking for control state could be extracted to improve readability.
+ private shouldShowErrors(): boolean { + return !(this.control?.ngControl?.untouched && + this.control?.ngControl?.pristine); + } + ngOnInit(): void { if (this.control) { this.control.errors$ .pipe( tap(errors => { this.errors = errors; }), takeUntil(this.destroy$) ) .subscribe(errors => { - if ( - errors && - !( - this.control.ngControl?.untouched && - this.control.ngControl?.pristine - ) - ) { + if (errors && this.shouldShowErrors()) { this.showErrors(errors); } else { this.showErrors({}); } }); } }sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts (1)
26-28: Improve type safety for errors stream.The errors Subject and Observable should have explicit typing.
- readonly errors$: Observable<ValidationErrors | null>; - private readonly errors = new Subject<ValidationErrors | null>(); + readonly errors$: Observable<ValidationErrors | null> = this.errors.asObservable(); + private readonly errors: Subject<ValidationErrors | null> = new Subject();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
sep490-frontend/src/app/modules/core/services/theme.service.ts(1 hunks)sep490-frontend/src/app/modules/dev/components/toolbox/toolbox.component.html(1 hunks)sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.html(2 hunks)sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.ts(3 hunks)sep490-frontend/src/app/modules/shared/components/form/abstract-form-component.ts(0 hunks)sep490-frontend/src/app/modules/shared/components/form/form-field-error/form-field-error.component.html(1 hunks)sep490-frontend/src/app/modules/shared/components/form/form-field-error/form-field-error.component.ts(1 hunks)sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts(1 hunks)sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts(1 hunks)sep490-frontend/src/app/modules/shared/shared.module.ts(2 hunks)sep490-frontend/tailwind.config.js(1 hunks)
💤 Files with no reviewable changes (1)
- sep490-frontend/src/app/modules/shared/components/form/abstract-form-component.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- sep490-frontend/src/app/modules/shared/components/form/form-field-error/form-field-error.component.html
- sep490-frontend/src/app/modules/shared/components/form/form-field-error/form-field-error.component.ts
- sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.ts
- sep490-frontend/src/app/modules/shared/shared.module.ts
- sep490-frontend/tailwind.config.js
- sep490-frontend/src/app/modules/dev/components/toolbox/toolbox.component.html
- sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.html
🧰 Additional context used
🪛 Biome (1.9.4)
sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts
[error] 43-43: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (4)
sep490-frontend/src/app/modules/core/services/theme.service.ts (2)
3-3: Verify theme migration impact across the application.The switch from Material to Aura theme is a significant change that could affect component styling and behavior throughout the application.
Run the following script to identify potential styling issues:
✅ Verification successful
Theme migration to PrimeNG is already complete and consistent
The codebase shows no traces of Material Design dependencies and demonstrates consistent PrimeNG usage across all components. The Aura theme import aligns with the existing architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for Material theme specific classes and components that might need updates # Search for Material-specific classes echo "Checking for Material-specific classes and components..." rg -l "mat-|material-" --type ts --type html # Search for PrimeNG component usage echo -e "\nChecking PrimeNG component usage..." rg -l "p-|primeng" --type ts --type htmlLength of output: 3628
7-22: Verify dark mode functionality with Aura theme.The removal of explicit color scheme definitions relies on Aura theme's built-in dark mode support. Please ensure that:
- All components transition correctly between light and dark modes
- Custom components using the red palette maintain proper contrast in both modes
Run the following script to identify components that might need dark mode testing:
✅ Verification successful
Dark mode functionality is properly implemented ✓
The ThemeService correctly handles dark mode transitions using Aura's built-in support. The service maintains both system preferences and user overrides, with proper dark mode selectors configured. The red palette customization doesn't impact the dark mode functionality as Aura automatically handles dark variants.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find components that might need dark mode verification # Search for color-sensitive components echo "Checking for color-sensitive components..." rg -l "bg-|text-|border-|shadow-" --type ts --type html # Search for custom theme classes echo -e "\nChecking for custom theme classes..." rg -l "class.*dark:|class.*light:" --type ts --type htmlLength of output: 1334
Script:
#!/bin/bash # Check theme service and Aura configuration echo "Checking theme service implementation..." rg -g "*.ts" -A 10 "class.*ThemeService" echo -e "\nChecking Aura theme usage..." rg -g "*.ts" "import.*@aura" -A 5 echo -e "\nChecking for dark mode related configuration..." rg -g "*.ts" "darkMode|colorScheme|mode:" -B 2 -A 2Length of output: 2510
sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts (1)
43-43: Avoid assignment in tap operator.The assignment within the tap operator makes the code harder to understand and maintain.
- tap(errors => (this.errors = errors)), + tap(errors => { + this.errors = errors; + }),🧰 Tools
🪛 Biome (1.9.4)
[error] 43-43: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts (1)
60-66: Improve type safety in event methods.Methods returning 'any' can lead to runtime errors. Consider adding proper return types.
- formSubmitEvent(): any { + formSubmitEvent(): Observable<Event> { return this.form.ngSubmit; } - controlStatusChanges(): any { + controlStatusChanges(): Observable<any> | undefined { return this.ngControl?.statusChanges; }
sep490-frontend/src/app/modules/shared/directives/form-field-error.directive.ts
Show resolved
Hide resolved
sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts
Show resolved
Hide resolved
sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts
Outdated
Show resolved
Hide resolved
thongdanghoang
force-pushed
the
feature/68-code-base-improvement
branch
from
bb4c7ec to
671cd19
Compare
thongdanghoang
force-pushed
the
feature/68-code-base-improvement
branch
from
671cd19 to
d4aa844
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
♻️ Duplicate comments (4)
sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts (4)
30-34: 🛠️ Refactor suggestionUse Renderer2 for DOM manipulation.
Add Renderer2 service to handle DOM manipulations in a way that's compatible with different rendering strategies.
constructor( @Optional() ngForm: NgForm, @Optional() formGroupDirective: FormGroupDirective, - private readonly el: ElementRef + private readonly el: ElementRef, + private readonly renderer: Renderer2 ) {
46-58: 🛠️ Refactor suggestionHandle potential race conditions and add error handling.
The current implementation has two issues:
- Initial error check might race with control initialization
- Missing error handling for merged observables
ngAfterViewInit(): void { if (this.ngControl) { - this.errors.next(this.ngControl.errors); + setTimeout(() => { + this.errors.next(this.ngControl?.errors ?? null); + }); merge(this.formSubmitEvent(), this.controlStatusChanges()) - .pipe(takeUntil(this.destroy$)) + .pipe( + takeUntil(this.destroy$), + catchError(error => { + console.error('Error in form validation stream:', error); + return EMPTY; + }) + ) .subscribe((): void => { this.errors.next(this.ngControl?.errors ?? null); this.updateDirtyStyle(this.ngControl?.errors); });
60-66: 🛠️ Refactor suggestionImprove type safety in event methods.
Methods returning 'any' can lead to runtime errors. Add proper return types.
-formSubmitEvent(): any { +formSubmitEvent(): Observable<Event> { return this.form.ngSubmit; } -controlStatusChanges(): any { +controlStatusChanges(): Observable<any> | undefined { return this.ngControl?.statusChanges; }
73-87: 🛠️ Refactor suggestionImprove DOM manipulation and element selection.
- Use Renderer2 for DOM manipulation instead of direct classList access
- Make element selection more specific to avoid potential issues
private updateDirtyStyle(errors: ValidationErrors | null | undefined): void { const element = this.findFormControlElement(); if (element) { if (errors && !(this.ngControl?.untouched && this.ngControl?.pristine)) { - element.classList.add('ng-dirty'); + this.renderer.addClass(element, 'ng-dirty'); } else { - element.classList.remove('ng-dirty'); + this.renderer.removeClass(element, 'ng-dirty'); } } } private findFormControlElement(): HTMLElement | null { const nativeElement = this.el.nativeElement; - return nativeElement.querySelector('[formControlName]'); + return nativeElement.querySelector('[formControlName]:not([formControlName=""])'); }
🧹 Nitpick comments (4)
sep490-frontend/src/app/modules/enterprise/components/create-user/create-user.component.ts (3)
57-60: LGTM! Clean validator implementation.Good approach using a custom validator for the buildings control. This centralizes the validation logic and makes it more maintainable.
Consider adding a type annotation for clarity:
- buildings: new FormControl<string[]>( + buildings: new FormControl<Array<string>>(
97-109: Consider enhancing the subscription logic.The filter for untouched and pristine states is good, but consider these improvements:
- Extract the filter condition to a named function for better readability
- Add error handling for the subscription
this.enterpriseUserStructure.scope.valueChanges .pipe( - filter( - (): boolean => - !( - this.enterpriseUserStructure.scope.untouched && - this.enterpriseUserStructure.scope.pristine - ) - ), + filter(this.isControlModified.bind(this)), takeUntil(this.destroy$) ) - .subscribe(value => { + .subscribe({ + next: (value) => { if (value === UserScope[UserScope.ENTERPRISE]) { this.enterpriseUserStructure.buildings.setValue([]); } + }, + error: (error) => { + console.error('Error in scope subscription:', error); + } + }); + private isControlModified(): boolean { + return !( + this.enterpriseUserStructure.scope.untouched && + this.enterpriseUserStructure.scope.pristine + ); + }
130-145: Enhance the validator implementation.The validator logic is correct but could be more robust and informative.
private buildingValidator(): ValidatorFn { return (control: AbstractControl): ValidationErrors | null => { - if (!this.enterpriseUserStructure) { + if (!this.enterpriseUserStructure?.scope?.value) { return null; } - if ( - this.enterpriseUserStructure.scope.value === - UserScope[UserScope.BUILDING] - ) { - if (control.value?.length === 0) { - return {required: true}; - } + + const isBuildingScope = this.enterpriseUserStructure.scope.value === UserScope[UserScope.BUILDING]; + const hasNoBuildings = !control.value?.length; + + if (isBuildingScope && hasNoBuildings) { + return { + required: { + message: 'At least one building must be selected for building scope users' + } + }; } return null; }; }sep490-frontend/src/app/modules/core/services/theme.service.ts (1)
Line range hint
23-40: Map primitive red colors to semantic error/danger colors.The red color palette is defined in primitive colors but not mapped to semantic colors. To improve maintainability and ensure consistent error styling:
- Add error/danger semantic colors mapping to the red palette
- Use semantic color tokens in components instead of primitive colors
Example addition:
semantic: { + error: { + 50: '#fef8f8', + 100: '#fbdfdf', + 200: '#f8c5c6', + 300: '#f5acad', + 400: '#f29293', + 500: '#ef797a', + 600: '#cb6768', + 700: '#a75555', + 800: '#834343', + 900: '#603031', + 950: '#3c1e1f' + }, primary: { // ... existing primary colors } }
🛑 Comments failed to post (1)
sep490-frontend/src/app/modules/shared/directives/error-messages.directive.ts (1)
15-15: 🛠️ Refactor suggestion
Add missing RxJS imports for proper error handling.
Add imports for
EMPTYandcatchErrorto handle potential errors in the observable streams.-import {Observable, Subject, merge, takeUntil} from 'rxjs'; +import {EMPTY, Observable, Subject, merge, takeUntil} from 'rxjs'; +import {catchError} from 'rxjs/operators';📝 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.import {EMPTY, Observable, Subject, merge, takeUntil} from 'rxjs'; import {catchError} from 'rxjs/operators';
Summary by CodeRabbit
Release Notes
New Features
Styling
Improvements