feat: POC for common style as variables files #1156
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
npm warn config production Use WalkthroughThis pull request introduces a new branding configuration system across multiple files. It adds style-specific configurations for different brands (Fyle and C1) and creates a centralized Changes
Suggested labels
Suggested reviewers
Possibly related PRs
Poem
✨ Finishing Touches
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 (
|
|
| @@ -2,8 +2,7 @@ | |||
| <div *ngIf="isLoading" class="tw-flex tw-justify-center tw-items-center tw-pt-80-px"> | |||
| <app-loader></app-loader> | |||
| </div> | |||
| <div *ngIf="!isLoading" class="configuration--contents tw-border-border-tertiary tw-mt-6" | |||
| [ngClass]="{'tw-mx-120-px tw-shadow-app-card': brandingConfig.brandId === 'fyle', 'tw-mx-60-px tw-shadow-shadow-level-1': brandingConfig.brandId === 'co'}"> | |||
| <div *ngIf="!isLoading" class="configuration--contents tw-border-border-tertiary tw-mt-6" [class]="brandingStyle.exportSettingsStyle"> | |||
There was a problem hiding this comment.
this should be [ngClass]="brandingStyle.exportSettingsStyle"
| export const brandingConfig: BrandingConfiguration = config as BrandingConfiguration; | ||
|
|
||
| export const c1Styles = { | ||
| exportSettingsStyle: 'tw-mx-60-px tw-shadow-shadow-level-1' |
![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)
src/app/branding/fyle-style-config.ts (1)
6-8: Consider using CSS custom properties for maintainability.Instead of hardcoding Tailwind classes, consider using CSS custom properties (variables) for values that might need to be adjusted across different themes or breakpoints.
export const fyleStyles = { - exportSettingsStyle: 'tw-mx-120-px tw-shadow-app-card' + exportSettingsStyle: 'tw-mx-[var(--fyle-export-margin)] tw-shadow-[var(--fyle-card-shadow)]' };src/app/branding/c1-style-config.ts (1)
1-8: Consider creating a shared configuration factory.The configuration structure is duplicated between Fyle and C1 styles. Consider creating a shared factory function to generate brand-specific configurations.
// shared-style-config.ts interface BrandStyleConfig { exportSettingsStyle: string; } function createBrandStyles(options: { margin: string; shadow: string; }): BrandStyleConfig { return { exportSettingsStyle: `tw-mx-${options.margin} tw-shadow-${options.shadow}` }; } // Usage in c1-style-config.ts export const c1Styles = createBrandStyles({ margin: '60-px', shadow: 'shadow-level-1' });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/branding/branding-config.ts(2 hunks)src/app/branding/c1-style-config.ts(1 hunks)src/app/branding/fyle-style-config.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.ts
🔇 Additional comments (2)
src/app/branding/fyle-style-config.ts (1)
4-4: Consider using type validation for config import.The type assertion
config as BrandingConfigurationdoesn't guarantee type safety at runtime. Consider adding runtime validation.src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-export-settings/qbd-direct-export-settings.component.html (1)
5-5: Clean implementation of dynamic styling.The use of
[class]="brandingStyle.exportSettingsStyle"is a clean way to apply brand-specific styles. The implementation correctly leverages the new branding configuration system.
|
|
||
| const styles = { | ||
| fyle: fyleStyles, | ||
| co: c1Styles | ||
| }; | ||
|
|
||
| // @ts-ignore | ||
| export const brandingStyle = styles[brandingConfig.brandId]; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove @ts-ignore and improve type safety.
The use of @ts-ignore masks potential type issues. Consider improving type safety:
const styles = {
fyle: fyleStyles,
co: c1Styles
};
-// @ts-ignore
-export const brandingStyle = styles[brandingConfig.brandId];
+type BrandId = typeof brandingConfig.brandId;
+export const brandingStyle = styles[brandingConfig.brandId as BrandId];Also consider using a type-safe lookup pattern:
const getBrandingStyle = (brandId: BrandId) => {
const style = styles[brandId];
if (!style) {
throw new Error(`Invalid brand ID: ${brandId}`);
}
return style;
};
Description
feat: POC for common style as variables files
Clickup
https://app.clickup.com/t/
Summary by CodeRabbit
Release Notes
New Features
Improvements
The changes improve the application's ability to customize visual styles across different brand configurations, providing a more tailored user experience.