feat: Refactor conditional styles in conguration componets to common branding styles #1166
Conversation
WalkthroughThe pull request introduces multiple modifications across various components, primarily focusing on the addition of a Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.ts
[failure] 72-72:
Trailing spaces not allowed
🪛 GitHub Actions: TypeScript Lint Check
src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.ts
[error] 72-72: Trailing spaces not allowed (no-trailing-spaces)
🪛 GitHub Actions: Unit Test
src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.ts
[error] 75-75: Duplicate identifier 'brandingStyle'. The variable name is already in use.
🔇 Additional comments (1)
src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.ts (1)
73-73: LGTM!The
brandingStyleproperty is correctly declared as readonly and follows Angular best practices.
| readonly brandingStyle = brandingStyle; | ||
|
|
||
| readonly brandingStyle = brandingStyle; |
There was a problem hiding this comment.
Remove duplicate brandingStyle declaration.
The brandingStyle property is declared twice, which causes a TypeScript compilation error.
Apply this diff to fix the issue:
readonly brandingStyle = brandingStyle;
-
- readonly brandingStyle = brandingStyle;📝 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.
| readonly brandingStyle = brandingStyle; | |
| readonly brandingStyle = brandingStyle; | |
| readonly brandingStyle = brandingStyle; |
🧰 Tools
🪛 GitHub Actions: Unit Test
[error] 75-75: Duplicate identifier 'brandingStyle'. The variable name is already in use.
| @@ -69,6 +69,8 @@ | |||
| private searchQuerySubject = new Subject<string>(); | |||
|
|
|||
| readonly brandingContent = brandingContent.exportLog; | |||
|
|
|||
There was a problem hiding this comment.
Remove trailing spaces.
The line contains trailing spaces which violates the linting rules and causes pipeline failures.
Apply this diff to fix the issue:
-
+🧰 Tools
🪛 GitHub Check: lint
[failure] 72-72:
Trailing spaces not allowed
🪛 GitHub Actions: TypeScript Lint Check
[error] 72-72: Trailing spaces not allowed (no-trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/app/shared/components/configuration/configuration-multi-select/configuration-multi-select.component.ts (1)
Line range hint
59-89: Remove commented-out drag-and-drop code.The file contains commented-out drag-and-drop related methods that should be removed to maintain code cleanliness.
-// DragStart(memo: string) { -// This.currentlyDragging = memo; -// } -// // On Drag End -// DragEnd() { -// This.currentlyDragging = null; -// } - -// // On Drop of Item to droppable area -// Drop() { -// If (this.currentlyDragging) { -// Let currentlyDraggingIndex = this.options.indexOf(this.currentlyDragging) -// This.selected = -// [...this.selected, this.currentlyDragging]; -// This.options = -// This.options.filter((val, i) => i != -// CurrentlyDraggingIndex); -// This.currentlyDragging = null; -// } -// } -// OnDragStart(index: number) { -// This.startIndex = index; -// } - -// OnDrop(dropIndex: number) { -// Const general = this.options[this.startIndex]; // get element -// This.options.splice(this.startIndex, 1); // delete from old position -// This.options.splice(dropIndex, 0, general); // add to new position -// }
🧹 Nitpick comments (5)
src/app/shared/components/configuration/configuration-select-field/configuration-select-field.component.ts (3)
Line range hint
31-32: Consider improving type safety for the options input.The
optionsinput accepts multiple array types using union types. Consider creating a common interface or type that these option types can implement/extend to improve type safety and maintainability.Example:
interface BaseOption { value: string; label?: string; } @Input() options: BaseOption[];
Line range hint
34-35: Address the TODO comment regarding app-specific types.The TODO comment indicates technical debt. Consider creating a plan to remove app-specific types and implement a more generic approach.
Would you like me to help create an issue to track this technical debt and propose a solution?
Line range hint
171-183: Optimize lifecycle hooks implementation.The
ngOnChangesimplementation could be improved:
- Consider using TypeScript's non-null assertion operator or optional chaining to avoid potential undefined errors
- The conditional logic could be simplified
ngOnChanges(changes: SimpleChanges): void { const control = this.form.get(this.formControllerName); if (control) { changes.isDisabled?.currentValue ? control.disable() : control.enable(); } }src/app/branding/fyle-style-config.ts (1)
11-25: Consider adding documentation for style properties.The new style properties are well-organized and follow consistent naming. However, adding JSDoc comments describing the purpose and usage of each property would improve maintainability.
Example documentation:
export const fyleStyles = { common: { // ... existing properties + /** Padding for input label text elements */ inputLabelTextStyle: 'tw-pt-8-px', + /** Common branding class for Fyle-specific styling */ configurationBrandingClass: 'fyle' }, // ... add similar documentation for other properties };src/app/shared/components/configuration/configuration-select-field/configuration-select-field.component.html (1)
10-11: LGTM! Consider simplifying the conditional logic.While the styling changes are good, the visibility conditions are quite complex. Consider breaking down the conditions into more readable chunks or moving the logic to the component.
Example refactor:
// In the component get showFirstSubLabel(): boolean { return !(this.exportConfigurationIconPath && this.appName === AppName.TRAVELPERK) && (this.subLabel !== undefined || !['reimbursableExportType', 'cccExportType', 'creditCardExportType'].includes(this.formControllerName)); }Then in the template:
-<h5 *ngIf="!(exportConfigurationIconPath && appName === AppName.TRAVELPERK ) && (subLabel !== undefined|| (formControllerName !== 'reimbursableExportType' && formControllerName!=='cccExportType' && formControllerName !== 'creditCardExportType'))" +<h5 *ngIf="showFirstSubLabel"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/app/branding/c1-style-config.ts(2 hunks)src/app/branding/fyle-style-config.ts(1 hunks)src/app/integrations/business-central/business-central-shared/business-central-import-settings/business-central-import-settings.component.html(1 hunks)src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.html(1 hunks)src/app/integrations/qbo/qbo-shared/qbo-import-settings/qbo-import-settings.component.html(1 hunks)src/app/shared/components/configuration/configuration-import-field/configuration-import-field.component.html(5 hunks)src/app/shared/components/configuration/configuration-import-field/configuration-import-field.component.ts(2 hunks)src/app/shared/components/configuration/configuration-multi-select/configuration-multi-select.component.html(1 hunks)src/app/shared/components/configuration/configuration-multi-select/configuration-multi-select.component.ts(2 hunks)src/app/shared/components/configuration/configuration-schedule-export/configuration-schedule-export.component.html(1 hunks)src/app/shared/components/configuration/configuration-schedule-export/configuration-schedule-export.component.ts(2 hunks)src/app/shared/components/configuration/configuration-select-field/configuration-select-field.component.html(1 hunks)src/app/shared/components/configuration/configuration-select-field/configuration-select-field.component.ts(2 hunks)src/app/shared/components/configuration/configuration-step-header/configuration-step-header.component.html(1 hunks)src/app/shared/components/configuration/configuration-step-header/configuration-step-header.component.ts(2 hunks)src/app/shared/components/configuration/configuration-text-field/configuration-text-field.component.html(1 hunks)src/app/shared/components/configuration/configuration-text-field/configuration-text-field.component.ts(2 hunks)src/app/shared/components/configuration/configuration-toggle-field/configuration-toggle-field.component.html(1 hunks)src/app/shared/components/configuration/configuration-toggle-field/configuration-toggle-field.component.ts(2 hunks)src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.html(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/integrations/intacct/intacct-main/intacct-export-log/intacct-completed-export-log/intacct-completed-export-log.component.ts
🔇 Additional comments (23)
src/app/shared/components/configuration/configuration-select-field/configuration-select-field.component.ts (1)
14-14: LGTM! Clean implementation of branding style.The addition of
brandingStyleimport and readonly property follows good practices for maintaining consistent branding across the application.Also applies to: 116-116
src/app/shared/components/configuration/configuration-text-field/configuration-text-field.component.ts (1)
3-3: LGTM! Changes align with standardization effort.The addition of
brandingStyleimport and property follows the consistent pattern being implemented across components for standardized styling.Also applies to: 28-29
src/app/branding/c1-style-config.ts (1)
11-26: LGTM! Style structure maintains consistency.The new style properties mirror the structure in
fyle-style-config.tswhile appropriately customizing values for C1 branding. This consistent structure across different branding configurations improves maintainability.src/app/shared/components/configuration/configuration-step-header/configuration-step-header.component.ts (1)
2-2: LGTM! Consistent implementation of branding styles.The addition of
brandingStylefollows the established pattern and maintains consistency with other branding-related properties in the component.Also applies to: 33-34
src/app/shared/components/configuration/configuration-schedule-export/configuration-schedule-export.component.ts (1)
3-3: LGTM! Clean implementation of branding style.The addition of
brandingStylealigns with the PR's objective to standardize styling approach.Also applies to: 42-43
src/app/shared/components/configuration/configuration-toggle-field/configuration-toggle-field.component.ts (1)
3-3: LGTM! Consistent implementation of branding style.The addition of
brandingStylefollows the same pattern as other branding configurations in the component.Also applies to: 48-49
src/app/shared/components/configuration/configuration-multi-select/configuration-multi-select.component.ts (1)
4-4: LGTM! Clean implementation of branding style.The addition of
brandingStyleis consistent with other components.Also applies to: 46-47
src/app/shared/components/configuration/configuration-import-field/configuration-import-field.component.ts (1)
3-3: LGTM! Clean implementation of branding style.The addition of
brandingStyleis consistent with other components and doesn't impact the existing import field logic.Also applies to: 113-114
src/app/shared/components/configuration/configuration-text-field/configuration-text-field.component.html (1)
11-12: LGTM! Improved styling consistency for password toggle icons.The change simplifies the styling logic by using a centralized branding class instead of conditional styling, making it more maintainable while preserving the toggle functionality.
src/app/shared/components/configuration/configuration-toggle-field/configuration-toggle-field.component.html (1)
11-11: LGTM! Consistent styling for input label text.The change appropriately uses the centralized brandingStyle for label text styling, improving maintainability.
src/app/shared/components/configuration/configuration-step-header/configuration-step-header.component.html (1)
6-6: LGTM! Semantic styling for configuration step header.Good use of semantically named styling from brandingStyle.configurationComponents, making the code's intent clear.
src/app/shared/components/configuration/configuration-multi-select/configuration-multi-select.component.html (1)
8-8: LGTM! Consistent sub-label styling.The change maintains consistency by using the same inputLabelTextStyle as other components.
src/app/shared/components/configuration/configuration-schedule-export/configuration-schedule-export.component.html (1)
40-40: LGTM! Simplified dropdown icon styling.The change effectively replaces complex conditional logic with a centralized branding class, maintaining consistency with other components.
src/app/integrations/business-central/business-central-shared/business-central-import-settings/business-central-import-settings.component.html (1)
29-31: LGTM! Good improvement in styling consistency.The change from conditional class binding to using
brandingStyle.common.inputLabelTextStylealigns well with the goal of standardizing styles across the application.src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-import-settings/qbd-direct-import-settings.component.html (1)
29-31: LGTM! Consistent implementation of branding style.The change maintains styling consistency across components while properly utilizing the branding content for labels.
src/app/integrations/qbo/qbo-shared/qbo-import-settings/qbo-import-settings.component.html (1)
29-31: LGTM! Maintains consistency with other import settings components.The implementation follows the same pattern as other components, ensuring a unified styling approach.
src/app/shared/components/dashboard/dashboard-error-section/dashboard-error-section.component.html (1)
27-29: LGTM! Excellent standardization of error resolution button styles.All error resolution buttons now consistently use
brandingStyle.dashboard.dashboardErrorResolveBtnText, ensuring uniform appearance across different error types.Also applies to: 51-53, 92-94
src/app/shared/components/configuration/configuration-import-field/configuration-import-field.component.html (6)
44-44: LGTM! Improved styling maintainability.The change from conditional class binding to using
brandingStyle.common.inputLabelTextStylealigns with the standardization effort and improves maintainability.
77-77: LGTM! Proper component-specific style organization.The use of
brandingStyle.configurationComponents.configurationImportSvgPaddingfollows good organization practices by keeping component-specific styles in a dedicated namespace.
104-105: LGTM! Clear separation of concerns in styling.The changes maintain a clear distinction between component-specific and common styles while preserving the conditional rendering logic.
125-125: LGTM! Consistent styling for dependent field elements.The changes maintain styling consistency across dependent field UI elements while properly organizing them under component-specific paths.
Also applies to: 128-128, 132-132
147-147: LGTM! Appropriate use of common styling.The use of
brandingStyle.common.configurationBrandingClassis appropriate for this Sage300-specific section, as the styling might be shared across similar configuration sections.
44-44: Verify the existence of all brandingStyle properties.While the changes look good, let's verify that all the required brandingStyle properties exist in the component:
Also applies to: 77-77, 104-105, 125-125, 128-128, 132-132, 147-147
✅ Verification successful
All brandingStyle properties are verified and exist in the codebase
The properties are used consistently across multiple components, following the established patterns for both common and configuration-specific styling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the brandingStyle property definition and its interface/type ast-grep --pattern 'readonly brandingStyle' # Search for the properties being used echo "Searching for brandingStyle properties usage..." properties=( "common.inputLabelTextStyle" "configurationComponents.configurationImportSvgPadding" "configurationComponents.configurationImportFieldDependentField" "common.configurationBrandingClass" "configurationComponents.configurationImportFieldDropDownText" "configurationComponents.configurationImportFieldDependentFieldSvg" ) for prop in "${properties[@]}"; do echo "Searching for: $prop" rg "brandingStyle\.$prop" -A 2 doneLength of output: 16025
src/app/branding/c1-style-config.ts
Outdated
| @@ -1,4 +1,5 @@ | |||
| import { BrandingConfiguration } from "../core/models/branding/branding-configuration.model"; | |||
| import { configuration } from "../integrations/intacct/intacct.fixture"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import.
The import of configuration from intacct.fixture appears to be unused in this file.
-import { configuration } from "../integrations/intacct/intacct.fixture";
DhaaraniCIT
changed the title
|
We are done with the converting all the style based on branding to a common branding style. This is the last PR for style |
src/app/branding/c1-style-config.ts
Outdated
| configurationImportSvgPadding: 'tw-pt-16-px', | ||
| configurationImportFieldDependentField: 'tw-pr-24-px tw-pl-[58px]', | ||
| configurationImportFieldDependentFieldSvg: 'tw-pt-42-px', | ||
| configurationImportFieldDropDownText: '!tw-font-600 tw-text-12-px', | ||
| configurationStepHeaderH3Text: 'tw-text-18-px' |
There was a problem hiding this comment.
"configuration" is not required
src/app/branding/c1-style-config.ts
Outdated
| }, | ||
| mapping: { | ||
| mappingHeaderBoxShadow: 'tw-shadow-shadow-level-1' | ||
| }, | ||
| configurationComponents: { |
There was a problem hiding this comment.
just configuration should do, "components" not required
|
Description
feat: converting condition style in ngStyle to a common branding style
Clickup
https://app.clickup.com/t/
Summary by CodeRabbit