feat: responsive grid layout + new tile layout#1141
Merged
JustARatherRidiculouslyLongUsername merged 2 commits intonew-app-tile-structurefrom Jan 17, 2025
Merged
feat: responsive grid layout + new tile layout#1141JustARatherRidiculouslyLongUsername merged 2 commits intonew-app-tile-structurefrom
JustARatherRidiculouslyLongUsername merged 2 commits intonew-app-tile-structurefrom
Conversation
Contributor
WalkthroughThe pull request introduces significant modifications to the integration applications section of the landing page. Key changes include transitioning from a flexbox to a grid layout for improved responsiveness, along with updates to the HTML structure that enhance clarity by explicitly displaying application types. The SCSS file has also been revised to support these layout changes, incorporating new classes and hover effects for better interactivity. Additionally, a new service and type definitions have been added to manage integrations effectively. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
🪧 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/app/integrations/landing-v2/landing-v2.component.html (1)
44-52: Consider extracting the app card into a reusable component.The app card structure is repeated multiple times with the same pattern. Consider creating a dedicated component to:
- Reduce code duplication
- Improve maintainability
- Ensure consistent structure
Example component structure:
@Component({ selector: 'app-integration-card', template: ` <div class="landing-v2--accounting-app" (click)="onClick()"> <img [src]="logoSrc" /> <div> <span class="landing-v2--accounting-app-name"> {{ name }} </span> <span class="landing-v2--accounting-app-type"> {{ type }} </span> </div> </div> ` }) export class IntegrationCardComponent { @Input() logoSrc: string; @Input() name: string; @Input() type: string; @Output() cardClick = new EventEmitter<void>(); onClick(): void { this.cardClick.emit(); } }Also applies to: 57-65, 69-77, 81-89, 93-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/integrations/landing-v2/landing-v2.component.html(1 hunks)src/app/integrations/landing-v2/landing-v2.component.scss(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: unit-test
🔇 Additional comments (4)
src/app/integrations/landing-v2/landing-v2.component.scss (3)
26-30: LGTM! Well-structured card layout.The updated styles create a consistent and visually appealing card layout with proper spacing and alignment.
34-35: LGTM! Improved text styling.The changes enhance readability with better text color and spacing.
37-38: LGTM! Well-designed type label styles.The new styles create appropriate visual hierarchy with the app name.
src/app/integrations/landing-v2/landing-v2.component.html (1)
39-40: LGTM! Well-implemented responsive grid.The grid layout provides excellent responsiveness with appropriate breakpoints from 2 to 5 columns.
| <span class="landing-v2--accounting-app-name !tw-pt-18-px"> | ||
| NetSuite | ||
| </span> | ||
| <img src="assets/logos/netsuite-logo.png" /> |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility improvements.
The implementation needs the following accessibility enhancements:
- Add alt text to images
- Add keyboard support
- Add ARIA labels
Example improvements:
-<img src="assets/logos/netsuite-logo.png" />
+<img
+ src="assets/logos/netsuite-logo.png"
+ alt="NetSuite logo"
+/>
-<div class="landing-v2--accounting-app" (click)="openAccountingIntegrationApp(AccountingIntegrationApp.NETSUITE)">
+<div
+ class="landing-v2--accounting-app"
+ (click)="openAccountingIntegrationApp(AccountingIntegrationApp.NETSUITE)"
+ (keydown.enter)="openAccountingIntegrationApp(AccountingIntegrationApp.NETSUITE)"
+ role="button"
+ tabindex="0"
+ aria-label="Open NetSuite integration">Also applies to: 57-57, 69-69, 81-81, 93-93, 106-106, 117-117, 129-129, 142-142, 156-156
Comment on lines
104
to
134
| <div *ngIf="isAppShown('QBD_DIRECT')" class="landing-v2--accounting-app" | ||
| (click)="openInAppIntegration(InAppIntegration.QBD_DIRECT)"> | ||
| <img src="assets/logos/quickbooks-logo.png" width="120px" height="60px" /> | ||
| <span class="landing-v2--accounting-app-name tw-gap-2 tw-px-4 tw-flex tw-items-center"> | ||
| QuickBooks Desktop (Web Connector) | ||
| <img src="assets/logos/quickbooks-logo.png" /> | ||
| <span class="landing-v2--accounting-app-name tw-items-center tw-gap-4"> | ||
| <div> | ||
| QuickBooks Desktop (Web Connector) | ||
| <div class="landing-v2--accounting-app-type">Accounting</div> | ||
| </div> | ||
| <app-badge [theme]="ThemeOption.DARK" text="Beta"></app-badge> | ||
| </span> | ||
| </div> | ||
| <div *ngIf="isAppShown('SAGE300')" class="landing-v2--accounting-app" | ||
| [ngClass]="{'tw-shadow-app-card': brandingConfig.brandId === 'fyle', 'tw-shadow-shadow-level-1': brandingConfig.brandId === 'co'}" | ||
| (click)="openInAppIntegration(InAppIntegration.SAGE300)"> | ||
| <img src="assets/logos/sage300-logo.svg" width="110px" height="60px" /> | ||
| <span class="landing-v2--accounting-app-name tw-gap-2"> | ||
| Sage 300 CRE | ||
| <img src="assets/logos/sage300-logo.svg" /> | ||
| <span class="landing-v2--accounting-app-name tw-items-center tw-gap-4"> | ||
| <div> | ||
| Sage 300 CRE | ||
| <div class="landing-v2--accounting-app-type">Accounting</div> | ||
| </div> | ||
| <app-badge *ngIf="!orgsToHideSage300BetaBadge.includes(org.fyle_org_id)" [theme]="ThemeOption.DARK" | ||
| text="Beta"></app-badge> | ||
| </span> | ||
| </div> | ||
| <div *ngIf="isAppShown('BUSINESS_CENTRAL')" | ||
| class="landing-v2--accounting-app" | ||
| [ngClass]="{'tw-shadow-app-card': brandingConfig.brandId === 'fyle', 'tw-shadow-shadow-level-1': brandingConfig.brandId === 'co'}" | ||
| <div *ngIf="isAppShown('BUSINESS_CENTRAL')" class="landing-v2--accounting-app" | ||
| (click)="openInAppIntegration(InAppIntegration.BUSINESS_CENTRAL)"> | ||
| <img src="assets/logos/BusinessCentral-logo.svg" class="tw-mt-[-20px]" width="50px" height="60px" /> | ||
| <span class="landing-v2--accounting-app-name tw-gap-2 tw-px-4 tw-flex tw-items-center"> | ||
| Dynamics 365 Business Central | ||
| <img src="assets/logos/BusinessCentral-logo.svg" /> | ||
| <span class="landing-v2--accounting-app-name tw-items-center tw-gap-4"> | ||
| <div> | ||
| Dynamics 365 Business Central | ||
| <div class="landing-v2--accounting-app-type">Accounting</div> | ||
| </div> |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistent structure in beta app cards.
The beta app cards have a different HTML structure compared to standard cards:
- Type label uses
divinstead ofspan - Different nesting structure
Apply this structure for consistency:
-<span class="landing-v2--accounting-app-name tw-items-center tw-gap-4">
- <div>
- QuickBooks Desktop (Web Connector)
- <div class="landing-v2--accounting-app-type">Accounting</div>
- </div>
- <app-badge [theme]="ThemeOption.DARK" text="Beta"></app-badge>
-</span>
+<div>
+ <span class="landing-v2--accounting-app-name tw-items-center tw-gap-4">
+ QuickBooks Desktop (Web Connector)
+ <app-badge [theme]="ThemeOption.DARK" text="Beta"></app-badge>
+ </span>
+ <span class="landing-v2--accounting-app-type">
+ Accounting
+ </span>
+</div>📝 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.
Suggested change
| <div *ngIf="isAppShown('QBD_DIRECT')" class="landing-v2--accounting-app" | |
| (click)="openInAppIntegration(InAppIntegration.QBD_DIRECT)"> | |
| <img src="assets/logos/quickbooks-logo.png" width="120px" height="60px" /> | |
| <span class="landing-v2--accounting-app-name tw-gap-2 tw-px-4 tw-flex tw-items-center"> | |
| QuickBooks Desktop (Web Connector) | |
| <img src="assets/logos/quickbooks-logo.png" /> | |
| <span class="landing-v2--accounting-app-name tw-items-center tw-gap-4"> | |
| <div> | |
| QuickBooks Desktop (Web Connector) | |
| <div class="landing-v2--accounting-app-type">Accounting</div> | |
| </div> | |
| <app-badge [theme]="ThemeOption.DARK" text="Beta"></app-badge> | |
| </span> | |
| </div> | |
| <div *ngIf="isAppShown('SAGE300')" class="landing-v2--accounting-app" | |
| [ngClass]="{'tw-shadow-app-card': brandingConfig.brandId === 'fyle', 'tw-shadow-shadow-level-1': brandingConfig.brandId === 'co'}" | |
| (click)="openInAppIntegration(InAppIntegration.SAGE300)"> | |
| <img src="assets/logos/sage300-logo.svg" width="110px" height="60px" /> | |
| <span class="landing-v2--accounting-app-name tw-gap-2"> | |
| Sage 300 CRE | |
| <img src="assets/logos/sage300-logo.svg" /> | |
| <span class="landing-v2--accounting-app-name tw-items-center tw-gap-4"> | |
| <div> | |
| Sage 300 CRE | |
| <div class="landing-v2--accounting-app-type">Accounting</div> | |
| </div> | |
| <app-badge *ngIf="!orgsToHideSage300BetaBadge.includes(org.fyle_org_id)" [theme]="ThemeOption.DARK" | |
| text="Beta"></app-badge> | |
| </span> | |
| </div> | |
| <div *ngIf="isAppShown('BUSINESS_CENTRAL')" | |
| class="landing-v2--accounting-app" | |
| [ngClass]="{'tw-shadow-app-card': brandingConfig.brandId === 'fyle', 'tw-shadow-shadow-level-1': brandingConfig.brandId === 'co'}" | |
| <div *ngIf="isAppShown('BUSINESS_CENTRAL')" class="landing-v2--accounting-app" | |
| (click)="openInAppIntegration(InAppIntegration.BUSINESS_CENTRAL)"> | |
| <img src="assets/logos/BusinessCentral-logo.svg" class="tw-mt-[-20px]" width="50px" height="60px" /> | |
| <span class="landing-v2--accounting-app-name tw-gap-2 tw-px-4 tw-flex tw-items-center"> | |
| Dynamics 365 Business Central | |
| <img src="assets/logos/BusinessCentral-logo.svg" /> | |
| <span class="landing-v2--accounting-app-name tw-items-center tw-gap-4"> | |
| <div> | |
| Dynamics 365 Business Central | |
| <div class="landing-v2--accounting-app-type">Accounting</div> | |
| </div> | |
| <div *ngIf="isAppShown('QBD_DIRECT')" class="landing-v2--accounting-app" | |
| (click)="openInAppIntegration(InAppIntegration.QBD_DIRECT)"> | |
| <img src="assets/logos/quickbooks-logo.png" /> | |
| <div> | |
| <span class="landing-v2--accounting-app-name tw-items-center tw-gap-4"> | |
| QuickBooks Desktop (Web Connector) | |
| <app-badge [theme]="ThemeOption.DARK" text="Beta"></app-badge> | |
| </span> | |
| <span class="landing-v2--accounting-app-type"> | |
| Accounting | |
| </span> | |
| </div> | |
| </div> | |
| <div *ngIf="isAppShown('SAGE300')" class="landing-v2--accounting-app" | |
| (click)="openInAppIntegration(InAppIntegration.SAGE300)"> | |
| <img src="assets/logos/sage300-logo.svg" /> | |
| <div> | |
| <span class="landing-v2--accounting-app-name tw-items-center tw-gap-4"> | |
| Sage 300 CRE | |
| <app-badge *ngIf="!orgsToHideSage300BetaBadge.includes(org.fyle_org_id)" [theme]="ThemeOption.DARK" | |
| text="Beta"></app-badge> | |
| </span> | |
| <span class="landing-v2--accounting-app-type"> | |
| Accounting | |
| </span> | |
| </div> | |
| </div> | |
| <div *ngIf="isAppShown('BUSINESS_CENTRAL')" class="landing-v2--accounting-app" | |
| (click)="openInAppIntegration(InAppIntegration.BUSINESS_CENTRAL)"> | |
| <img src="assets/logos/BusinessCentral-logo.svg" /> | |
| <div> | |
| <span class="landing-v2--accounting-app-name tw-items-center tw-gap-4"> | |
| Dynamics 365 Business Central | |
| <div class="landing-v2--accounting-app-type">Accounting</div> | |
| </div> |
ashwin1111
approved these changes
Jan 7, 2025
* fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
|
JustARatherRidiculouslyLongUsername
merged commit f3a45a9
into
new-app-tile-structure
5 checks passed
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
src/app/integrations/landing-v2/landing-v2.component.html (1)
Line range hint
15-38: Add accessibility improvements to tab navigation.The tab navigation needs accessibility enhancements:
- Add keyboard navigation support
- Add proper ARIA roles and attributes
-<div class="tw-flex tw-text-14-px tw-border-y tw-border-separator"> +<div class="tw-flex tw-text-14-px tw-border-y tw-border-separator" role="tablist"> <div class="landing-v2--tab" [ngClass]="{'tw-text-menu-inactive-text-color': !integrationTabs.ALL}" - (click)="switchView(IntegrationsView.ALL)"> + (click)="switchView(IntegrationsView.ALL)" + (keydown.enter)="switchView(IntegrationsView.ALL)" + role="tab" + [attr.aria-selected]="integrationTabs.ALL" + tabindex="0"> All <p *ngIf="integrationTabs.ALL" class="landing-v2--active-tag"></p> </div> <!-- Apply similar changes to other tabs -->
♻️ Duplicate comments (2)
src/app/integrations/landing-v2/landing-v2.component.html (2)
41-59: 🛠️ Refactor suggestionAdd accessibility attributes to app cards.
Interactive elements need proper accessibility attributes:
- Add alt text to images
- Add keyboard support and ARIA labels to clickable cards
-<div class="landing-v2--accounting-app" +<div class="landing-v2--accounting-app" + role="button" + tabindex="0" + (keydown.enter)="openAccountingIntegrationApp(AccountingIntegrationApp.NETSUITE)" + aria-label="Open NetSuite integration" (click)="openAccountingIntegrationApp(AccountingIntegrationApp.NETSUITE)"> <div class="tw-flex tw-justify-between tw-items-center"> - <img src="assets/logos/netsuite-logo-new.png" /> + <img + src="assets/logos/netsuite-logo-new.png" + alt="NetSuite logo" + />
149-154: 🛠️ Refactor suggestionFix inconsistent structure in beta app cards.
The beta app cards (QuickBooks Desktop Web Connector, Sage 300 CRE, and Business Central) have inconsistent HTML structure compared to other cards.
Apply consistent structure across all cards:
-<span class="landing-v2--accounting-app-name tw-items-center tw-gap-4"> - <div> - QuickBooks Desktop (Web Connector) - <div class="landing-v2--accounting-app-type">Accounting</div> - </div> - <app-badge [theme]="ThemeOption.DARK" text="Beta"></app-badge> -</span> +<div> + <span class="landing-v2--accounting-app-name"> + QuickBooks Desktop (Web Connector) + <app-badge [theme]="ThemeOption.DARK" text="Beta"></app-badge> + </span> + <span class="landing-v2--accounting-app-type"> + Accounting + </span> +</div>Also applies to: 167-173, 186-191
🧹 Nitpick comments (6)
src/app/integrations/integrations-routing.module.ts (1)
17-17: Maintain consistent route naming convention.The route naming is inconsistent across the module. Some routes use hyphens (e.g.,
bamboo-hr), while others use underscores (e.g.,landing_v2). Consider adopting a consistent naming convention for all routes.- path: 'landing_v2', + path: 'landing-v2',src/app/integrations/landing-v2/landing-v2.component.ts (2)
35-37: Consider initializing the connectedApps array.The
connectedAppsproperty should be initialized as an empty array to avoid potential undefined errors when accessing it before the API response.- private connectedApps: IntegrationAppKey[]; + private connectedApps: IntegrationAppKey[] = [];
80-91: Consider using an enum for TPA names to prevent typos.The string literals in
tpaNameToIntegrationKeyMapcould be replaced with an enum to prevent typos and improve maintainability.enum TPAName { NETSUITE = 'Fyle Netsuite Integration', SAGE_INTACCT = 'Fyle Sage Intacct Integration', // ... other names }src/app/integrations/landing-v2/landing-v2.component.scss (2)
26-35: Consider adding focus styles for accessibility.While the hover styles are good, keyboard navigation users would benefit from focus styles.
&--accounting-app { @apply tw-h-full tw-flex tw-flex-col tw-p-4 tw-gap-4 tw-justify-evenly tw-cursor-pointer tw-rounded-8-px tw-border tw-border-separator tw-bg-white; - @apply tw-transition-shadow; + @apply tw-transition-shadow tw-outline-none; img { @apply tw-h-[40px]; } .btn-connect { @apply tw-text-14-px tw-hidden; } + + &:focus-visible { + @apply tw-ring-2 tw-ring-primary tw-ring-offset-2; + } }
67-71: Consider using Tailwind's shadow utilities.Instead of using custom box-shadow, consider using Tailwind's built-in shadow utilities for consistency.
.landing-v2--accounting-app:hover { - box-shadow: 0px 4px 4px 0px rgba(44, 48, 78, 0.10); + @apply tw-shadow-md; .btn-connect { @apply tw-block; } }src/app/integrations/landing-v2/landing-v2.component.html (1)
46-50: Add loading state to connect buttons.The connect buttons should indicate loading state while the connection is being established to prevent multiple clicks.
-<button class="btn-connect">Connect</button> +<button + class="btn-connect" + [disabled]="isConnecting" + [attr.aria-busy]="isConnecting"> + {{ isConnecting ? 'Connecting...' : 'Connect' }} +</button>Also applies to: 66-70, 85-89, 104-108, 123-127, 142-147, 161-165, 180-184, 200-204, 221-225
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
src/assets/logos/bamboo-hr-logo.pngis excluded by!**/*.pngsrc/assets/logos/intacct-logo-new.pngis excluded by!**/*.pngsrc/assets/logos/netsuite-logo-new.pngis excluded by!**/*.pngsrc/assets/logos/sage300-logo.pngis excluded by!**/*.pngsrc/assets/logos/travelperk-logo.pngis excluded by!**/*.pngsrc/assets/logos/xero-logo-new.pngis excluded by!**/*.png
📒 Files selected for processing (9)
src/app/core/models/enum/enum.model.ts(1 hunks)src/app/core/models/integrations/integrations.model.ts(1 hunks)src/app/core/services/common/integrations.service.spec.ts(1 hunks)src/app/core/services/common/integrations.service.ts(1 hunks)src/app/integrations/integrations-routing.module.ts(1 hunks)src/app/integrations/landing-v2/landing-v2.component.html(2 hunks)src/app/integrations/landing-v2/landing-v2.component.scss(2 hunks)src/app/integrations/landing-v2/landing-v2.component.ts(6 hunks)src/app/shared/components/core/badge/badge.component.scss(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: unit-test
🔇 Additional comments (6)
src/app/integrations/landing-v2/landing-v2.component.ts (2)
18-18: LGTM!The import of
IntegrationsServiceis appropriate for the new connection status feature.
168-170: LGTM!The
isAppConnectedmethod implementation is clean and straightforward.src/app/core/models/enum/enum.model.ts (1)
905-906: LGTM!The addition of
SUCCESStoThemeOptionenum is appropriate for the new success theme styling.src/app/shared/components/core/badge/badge.component.scss (1)
25-28: LGTM!The
.theme-successclass follows the established pattern and uses appropriate Tailwind CSS utilities for consistent styling.src/app/integrations/landing-v2/landing-v2.component.scss (1)
39-44: LGTM!The text styling for app name and type creates a clear visual hierarchy.
src/app/integrations/landing-v2/landing-v2.component.html (1)
39-40: LGTM! Well-structured responsive grid layout.The grid layout implementation with Tailwind CSS is clean and properly handles different screen sizes with appropriate column counts.
Comment on lines
+8
to
+15
| beforeEach(() => { | ||
| TestBed.configureTestingModule({}); | ||
| service = TestBed.inject(IntegrationsService); | ||
| }); | ||
|
|
||
| it('should be created', () => { | ||
| expect(service).toBeTruthy(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add test coverage for getIntegrations method.
The test suite only verifies service creation. Missing critical test coverage for:
- Successful API responses
- Error handling
- Base URL configuration
- API endpoint construction
Here's a suggested test structure:
it('should fetch integrations successfully', (done) => {
const mockIntegrations = [{ id: 1, org_id: '123', /* ... */ }];
spyOn(apiService, 'get').and.returnValue(of(mockIntegrations));
service.getIntegrations().subscribe(result => {
expect(result).toEqual(mockIntegrations);
expect(apiService.get).toHaveBeenCalledWith('/integrations/', {});
done();
});
});
it('should handle API errors', (done) => {
spyOn(apiService, 'get').and.returnValue(throwError(() => new Error('API Error')));
service.getIntegrations().subscribe({
error: (error) => {
expect(error.message).toBe('API Error');
done();
}
});
});|
|
||
| import { IntegrationsService } from './integrations.service'; | ||
|
|
||
| xdescribe('IntegrationsService', () => { |
Contributor
There was a problem hiding this comment.
Remove xdescribe to enable test execution.
The test suite is currently disabled with xdescribe. This prevents the tests from being executed as part of the test suite.
-xdescribe('IntegrationsService', () => {
+describe('IntegrationsService', () => {📝 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.
Suggested change
| xdescribe('IntegrationsService', () => { | |
| describe('IntegrationsService', () => { |
Comment on lines
+18
to
+21
| getIntegrations(): Observable<Integration[]> { | ||
| this.helper.setBaseApiURL(AppUrl.INTEGRATION); | ||
| return this.apiService.get(`/integrations/`, {}); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and type safety for API response.
The getIntegrations method lacks error handling and type safety for the API response. Consider:
- Adding error handling with catchError operator
- Enforcing response type with HTTP options
getIntegrations(): Observable<Integration[]> {
this.helper.setBaseApiURL(AppUrl.INTEGRATION);
- return this.apiService.get(`/integrations/`, {});
+ return this.apiService.get<Integration[]>(`/integrations/`, {
+ responseType: 'json'
+ }).pipe(
+ catchError(error => {
+ console.error('Failed to fetch integrations:', error);
+ return throwError(() => error);
+ })
+ );
}Don't forget to import:
import { catchError, throwError } from 'rxjs';
Comment on lines
+3
to
+12
| export type Integration = { | ||
| id: number; | ||
| org_id: string; | ||
| org_name: string; | ||
| tpa_id: string; | ||
| tpa_name: string; | ||
| type: string; | ||
| is_active: boolean; | ||
| is_beta: boolean; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and add documentation for Integration type.
- The
typefield should be an enum instead of string to prevent invalid values - Missing JSDoc documentation for the type and its fields
- Consider adding validation for sensitive fields
+/**
+ * Represents an integration between the application and a third-party service
+ */
export type Integration = {
+ /** Unique identifier for the integration */
id: number;
+ /** Organization identifier */
org_id: string;
+ /** Organization name */
org_name: string;
+ /** Third-party application identifier */
tpa_id: string;
+ /** Third-party application name */
tpa_name: string;
- type: string;
+ /** Type of integration */
+ type: IntegrationType;
+ /** Whether the integration is currently active */
is_active: boolean;
+ /** Whether the integration is in beta testing phase */
is_beta: boolean;
}
+/**
+ * Supported integration types
+ */
+export enum IntegrationType {
+ ACCOUNTING = 'ACCOUNTING',
+ HRMS = 'HRMS',
+ TRAVEL = 'TRAVEL'
+}Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+241
to
+248
| private storeConnectedApps() { | ||
| this.integrationService.getIntegrations().subscribe(integrations => { | ||
| const tpaNames = integrations.map(integration => integration.tpa_name); | ||
| const connectedApps = tpaNames.map(tpaName => this.tpaNameToIntegrationKeyMap[tpaName]); | ||
|
|
||
| this.connectedApps = connectedApps; | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the API call.
The storeConnectedApps method should handle potential API errors to prevent the application from breaking.
private storeConnectedApps() {
this.integrationService.getIntegrations().subscribe(
- integrations => {
+ {
+ next: (integrations) => {
const tpaNames = integrations.map(integration => integration.tpa_name);
const connectedApps = tpaNames.map(tpaName => this.tpaNameToIntegrationKeyMap[tpaName]);
this.connectedApps = connectedApps;
+ },
+ error: (error) => {
+ console.error('Failed to fetch integrations:', error);
+ // Handle error appropriately, maybe show a toast notification
+ }
}
);
}📝 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.
Suggested change
| private storeConnectedApps() { | |
| this.integrationService.getIntegrations().subscribe(integrations => { | |
| const tpaNames = integrations.map(integration => integration.tpa_name); | |
| const connectedApps = tpaNames.map(tpaName => this.tpaNameToIntegrationKeyMap[tpaName]); | |
| this.connectedApps = connectedApps; | |
| }); | |
| } | |
| private storeConnectedApps() { | |
| this.integrationService.getIntegrations().subscribe( | |
| { | |
| next: (integrations) => { | |
| const tpaNames = integrations.map(integration => integration.tpa_name); | |
| const connectedApps = tpaNames.map(tpaName => this.tpaNameToIntegrationKeyMap[tpaName]); | |
| this.connectedApps = connectedApps; | |
| }, | |
| error: (error) => { | |
| console.error('Failed to fetch integrations:', error); | |
| // Handle error appropriately, maybe show a toast notification | |
| } | |
| } | |
| ); | |
| } |
JustARatherRidiculouslyLongUsername
added a commit
that referenced
this pull request
Jan 17, 2025
* feat: new app tile structure + remove extra content * feat: responsive grid layout + new tile layout (#1141) * feat: responsive grid layout + new tile layout * feat: add CTA and shadow animation on tile hover (#1142) * fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
JustARatherRidiculouslyLongUsername
added a commit
that referenced
this pull request
Jan 17, 2025
* feat: create new landing page and add header and tab switcher * feat: new app tile structure + remove extra content (#1140) * feat: new app tile structure + remove extra content * feat: responsive grid layout + new tile layout (#1141) * feat: responsive grid layout + new tile layout * feat: add CTA and shadow animation on tile hover (#1142) * fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
JustARatherRidiculouslyLongUsername
added a commit
that referenced
this pull request
Jan 17, 2025
* feat: create new landing page and add header and tab switcher * feat: new app tile structure + remove extra content (#1140) * feat: new app tile structure + remove extra content * feat: responsive grid layout + new tile layout (#1141) * feat: responsive grid layout + new tile layout * feat: add CTA and shadow animation on tile hover (#1142) * fix: update images * feat: add CTA and shadow animation on tile hover * feat: UI for the new "Conencted" badge (#1144) * feat: UI for the new "Conencted" badge * feat: fetch data from integrations API and show connected apps (#1143) * feat: fetch data from integrations API and show connected apps * fix: update route name and keep `exposeC1Apps` (#1145) * fix: update route name and keep `exposeC1Apps` * fix: use new logos in landing-v2 only Use original images everywhere else. * fix: incorrect URL for `/integrations` call and rendering errors (#1149)
This was referenced Jan 17, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clickup
https://app.clickup.com/t/86cxhz07t
Summary by CodeRabbit
New Features
Style
Bug Fixes