fix: QA fixes after design team testing #1092
Conversation
WalkthroughThe pull request introduces modifications primarily to the HTML templates of various components within the QBD integration. Key changes include the addition of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (11)
src/app/shared/components/configuration/configuration-confirmation-dialog/configuration-confirmation-dialog.component.html (2)
14-14: Consider simplifying the close icon class logicThe current ngClass implementation combines multiple conditions in the template. Consider moving this logic to the component class for better maintainability.
Example refactor:
- <i [ngClass]="{'pi pi-times fyle' : brandingConfig.brandId === 'fyle' && appName!==AppName.QBD_DIRECT, 'pi pi-times fyle qbd' : brandingConfig.brandId === 'fyle' && appName===AppName.QBD_DIRECT, 'pi pi-times co' : brandingConfig.brandId !== 'fyle'}" class="tw-cursor-pointer" style="font-size: 12px" (click)="acceptWarning(false)"></i> + <i [ngClass]="getCloseIconClasses()" class="tw-cursor-pointer" style="font-size: 12px" (click)="acceptWarning(false)"></i>Add to component class:
getCloseIconClasses(): string { const baseClasses = 'pi pi-times'; if (this.brandingConfig.brandId !== 'fyle') { return `${baseClasses} co`; } return `${baseClasses} fyle${this.appName === this.AppName.QBD_DIRECT ? ' qbd' : ''}`; }
30-32: Consider adding a tooltip for truncated URLsWhile truncating long URLs helps maintain the layout, it might hide important information from users. Consider adding a tooltip to show the full URL on hover.
- <p class="tw-text-size-10 tw-text-text-disable tw-font-400 tw-max-w-[400px] tw-truncate"> + <p class="tw-text-size-10 tw-text-text-disable tw-font-400 tw-max-w-[400px] tw-truncate" [pTooltip]="brandingKbArticles.onboardingArticles.QBD_DIRECT.HELPER_ARTICLE">src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-landing/qbd-direct-onboarding-landing.component.html (1)
6-6: LGTM! Consider extracting the header text to a constant.The text change from "QuickBooks Online" to "Fyle-QuickBooks Desktop" improves clarity. However, consider moving this string to a constant in the component or a shared configuration file to maintain consistency and ease future updates.
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.ts (1)
Line range hint
17-31: Consider improving type safety for EventEmitter outputs.The event emitters could benefit from explicit type parameters to improve type safety.
- @Output() nextStep = new EventEmitter(); + @Output() nextStep = new EventEmitter<void>(); - @Output() retryClick = new EventEmitter(); + @Output() retryClick = new EventEmitter<void>(); - @Output() manualDownload = new EventEmitter(); + @Output() manualDownload = new EventEmitter<void>();src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.ts (1)
35-36: LGTM! Consider adding type safety to the EventEmitterThe addition of the manual download event emitter follows Angular best practices.
Consider adding void type to the EventEmitter for better type safety:
-@Output() manualDownload = new EventEmitter(); +@Output() manualDownload = new EventEmitter<void>();src/styles.scss (1)
194-194: Consider extracting dialog styles into a themed component.The dialog content styles are using Tailwind utilities with
!important. This approach might make it difficult to maintain consistent dialog styling across the application.Consider:
- Creating a themed dialog component with consistent styles
- Using CSS custom properties for configurable values
-.p-dialog .p-dialog-content { - @apply tw-p-0 tw-rounded-border-radius-2xs #{!important}; -} +:root { + --dialog-padding: 0; + --dialog-border-radius: var(--border-radius-2xs); +} + +.p-dialog .p-dialog-content { + padding: var(--dialog-padding); + border-radius: var(--dialog-border-radius); +}src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.html (5)
1-1: Consider adding ARIA attributes for better accessibilityWhile the styling changes look good, consider enhancing accessibility by adding appropriate ARIA attributes to the step indicator and header.
-<div class="tw-border tw-rounded-border-radius-2xs tw-border-border-tertiary" [ngClass]="{'tw-opacity-50': !showSection}"> +<div class="tw-border tw-rounded-border-radius-2xs tw-border-border-tertiary" [ngClass]="{'tw-opacity-50': !showSection}" role="region" aria-label="Data Sync Configuration"> <div class="tw-p-24-px"> <div class="tw-flex tw-items-center tw-justify-between"> <div class="tw-flex tw-items-center tw-justify-start"> - <div class="tw-w-20-px tw-h-20-px tw-rounded-border-radius-2xs tw-bg-bg-secondary tw-text-white tw-text-14-px tw-font-500 tw-text-center tw-mr-12-px"> + <div class="tw-w-20-px tw-h-20-px tw-rounded-border-radius-2xs tw-bg-bg-secondary tw-text-white tw-text-14-px tw-font-500 tw-text-center tw-mr-12-px" role="heading" aria-level="2"> 3 </div>Also applies to: 5-10
Line range hint
13-29: Add trackBy function to optimize ngFor performanceThe ngFor directive would benefit from a trackBy function to improve rendering performance when the qbdFields array changes.
-<div *ngFor="let field of qbdFields; let i = index" +<div *ngFor="let field of qbdFields; let i = index; trackBy: trackByField"Add to your component:
trackByField(index: number, field: any): string { return field.attribute_type; }
Line range hint
30-32: Consider clarifying the auto-refresh messageThe current message about automatic refresh in 5 minutes might benefit from being more specific about when this countdown starts.
-[infoText]="'The values displayed here are synced from QuickBooks Desktop to Fyle. If you notice discrepancies in the synced values, you can continue onboarding; the sync will refresh automatically in 5 minutes to capture any missed values.'" +[infoText]="'The values displayed here are synced from QuickBooks Desktop to Fyle. If you notice discrepancies in the synced values, you can continue onboarding; the sync will automatically refresh within 5 minutes from now to capture any missed values.'"
Line range hint
33-36: Improve loading state UXThe loading state could be enhanced to:
- Use min-height instead of fixed height to prevent layout shifts
- Provide more descriptive loading text
-<div *ngIf="isLoading && showSection" class="tw-flex tw-flex-col tw-justify-center tw-items-center tw-h-[400px]"> +<div *ngIf="isLoading && showSection" class="tw-flex tw-flex-col tw-justify-center tw-items-center tw-min-h-[400px]"> <app-loader styleClass="spinner-30 tw-top-2-px"></app-loader> - <span class="tw-text-14-px tw-font-400 tw-text-text-primary">Loading</span> + <span class="tw-text-14-px tw-font-400 tw-text-text-primary">Syncing QuickBooks Desktop data...</span>
Property naming is inconsistent with its usage
The verification confirms that
isCTAEnabledis used incorrectly in the template. In the QbdDirectDataSyncComponent:
- The property is named
isCTAEnabledsuggesting it enables the button- But in the template, it's bound to
[isButtonDisabled]="isCTAEnabled"which does the oppositeThis creates confusion as:
- In QbdDirectSetupConnectionComponent, it's used correctly as
[isButtonDisabled]="!isCTAEnabled"- But in QbdDirectDataSyncComponent, it's used incorrectly as
[isButtonDisabled]="isCTAEnabled"To fix this:
- Either rename the property to
isCTADisabledto match its usage- Or fix the template binding to
[isButtonDisabled]="!isCTAEnabled"to maintain consistency🔗 Analysis chain
Line range hint
37-39: Verify the isCTAEnabled property usageThe property name
isCTAEnabledseems counterintuitive when used withisButtonDisabled. Consider renaming for clarity.
Let me gather more information about the component class to understand the complete context.
Let me search for the data sync component implementation to complete the analysis.
Let me try a different approach to find the data sync component implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the isCTAEnabled property definition and usage ast-grep --pattern 'class $_ { $$$ isCTAEnabled $$$ }' # Search for related template usage rg "isCTAEnabled" --type htmlLength of output: 1110
Script:
#!/bin/bash # Search for the component class implementation ast-grep --pattern 'class QbdDirectDataSyncComponent { $$$ }' # Also search for related component classes to understand the pattern ast-grep --pattern 'class QbdDirectSetupConnectionComponent { $$$ }'Length of output: 9685
Script:
#!/bin/bash # Search for the data sync component implementation with a more flexible pattern ast-grep --pattern 'class $_ { $$$ isCTAEnabled $$$ }' src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.tsLength of output: 167
Script:
#!/bin/bash # Search for the file content directly rg -A 20 "export class QbdDirectDataSyncComponent" src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.tsLength of output: 618
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-configuration/qbd-direct-configuration.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-export-log/qbd-direct-export-log.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-mapping/qbd-direct-mapping.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-landing/qbd-direct-onboarding-landing.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-pre-requisite/qbd-direct-onboarding-pre-requisite.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-pre-requisite/qbd-direct-onboarding-pre-requisite.component.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.html(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.html(2 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.ts(1 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.html(4 hunks)src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.ts(2 hunks)src/app/shared/components/configuration/configuration-confirmation-dialog/configuration-confirmation-dialog.component.html(2 hunks)src/app/shared/components/helper/app-landing-page-body/app-landing-page-body.component.html(1 hunks)src/app/shared/components/helper/app-landing-page-header/app-landing-page-header.component.html(2 hunks)src/app/shared/components/helper/onboarding-done/onboarding-done.component.html(1 hunks)src/app/shared/components/helper/onboarding-stepper/onboarding-stepper.component.html(1 hunks)src/styles.scss(2 hunks)
✅ Files skipped from review due to trivial changes (7)
- src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-dashboard/qbd-direct-dashboard.component.html
- src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-configuration/qbd-direct-configuration.component.html
- src/app/shared/components/helper/onboarding-stepper/onboarding-stepper.component.html
- src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-export-log/qbd-direct-export-log.component.html
- src/app/integrations/qbd-direct/qbd-direct-main/qbd-direct-mapping/qbd-direct-mapping.component.html
- src/app/shared/components/helper/onboarding-done/onboarding-done.component.html
- src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.html
🔇 Additional comments (13)
src/app/shared/components/configuration/configuration-confirmation-dialog/configuration-confirmation-dialog.component.html (1)
9-12: LGTM! Enhanced visual distinction for QBD_DIRECT warnings
The styling changes appropriately differentiate QBD_DIRECT warnings with rounded corners and danger-themed colors, improving the user experience through better visual hierarchy.
src/app/shared/components/helper/app-landing-page-body/app-landing-page-body.component.html (1)
4-10: Verify design system color and font-weight changes.
The changes include:
- Text color change from
tw-text-text-secondarytotw-text-text-muted - Addition of
tw-font-500to the link
Please ensure these changes align with the design system's color palette and typography guidelines.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient evidence to verify the design system changes:
Color and font-weight changes align with design system
The changes are consistent with the design system's usage across the application:
tw-text-text-mutedis widely used for secondary/hint text across multiple componentstw-font-500is consistently used for links and interactive elements to improve visibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for consistent usage of these color and font-weight classes
echo "Checking color class usage:"
rg "tw-text-text-muted" --type html
echo "Checking font weight usage:"
rg "tw-font-500" --type html
Length of output: 51148
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-download-file/qbd-direct-download-file.component.ts (1)
12-12: Verify removal of QbdDirectSharedComponent.
The QbdDirectSharedComponent has been removed from imports. Please confirm this doesn't affect any shared functionality or styling.
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.ts (1)
78-81: LGTM! Method implementation is clean and focused
The onManualDownload method implementation is straightforward and follows the single responsibility principle.
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-pre-requisite/qbd-direct-onboarding-pre-requisite.component.html (1)
24-24:
Verify HTML sanitization for innerHTML binding
While using innerHTML allows for rich text formatting in captions, it could potentially expose the application to XSS attacks if the content isn't properly sanitized.
Let's check if proper sanitization is implemented:
If sanitization is not implemented, consider either:
- Using Angular's built-in DomSanitizer:
constructor(private sanitizer: DomSanitizer) {
this.step.caption = this.sanitizer.bypassSecurityTrustHtml(caption);
}- Or using a safe HTML rendering library like ngx-markdown if the captions contain markdown content.
✅ Verification successful
HTML content is statically defined and safe to use with innerHTML
The captions are hardcoded strings in the component with only trusted HTML content (a simple anchor tag with target="_blank"). The content is not dynamically generated or received from external sources, making it safe to use with innerHTML binding in this specific case.
- The caption content is statically defined in the component:
caption: `<a href='https://developer.intuit.com/app/developer/qbdesktop/docs/get-started/get-started-with-quickbooks-web-connector' target="_blank" class=" tw-underline !tw-underline-offset-1" >Download</a> and install...`
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if DomSanitizer is used in the component
rg -l "DomSanitizer" src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-pre-requisite/
# Check caption content initialization
ast-grep --pattern 'preRequisitesteps = $_'
Length of output: 157
Script:
#!/bin/bash
# Let's check the component file for sanitization and caption initialization
rg -A 5 "caption" src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-pre-requisite/qbd-direct-onboarding-pre-requisite.component.ts
# Check if DomSanitizer is imported and used (with more context)
rg -A 5 "import.*DomSanitizer|sanitizer" src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-pre-requisite/qbd-direct-onboarding-pre-requisite.component.ts
# Check where the steps data is defined/initialized
rg -A 10 "steps.*=|preRequisiteSteps.*=" src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-pre-requisite/
Length of output: 5678
src/app/integrations/qbd-direct/qbd-direct-onboarding/qbd-direct-onboarding-connector/qbd-direct-onboarding-connector.component.html (1)
36-37: Verify triggerManualDownload implementation
The manual download event binding has been properly added, but let's ensure the handler method is properly implemented.
Let's check the implementation of triggerManualDownload:
✅ Verification successful
triggerManualDownload method is properly implemented
The method is correctly implemented in the component with proper XML file handling and download functionality using Blob and URL creation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for triggerManualDownload method implementation
ast-grep --pattern 'triggerManualDownload() {
$$$
}'
Length of output: 56
Script:
#!/bin/bash
# Let's try a different pattern to search for the method, considering potential TypeScript syntax
rg "triggerManualDownload\(" --type ts -A 5
Length of output: 2024
src/app/shared/components/helper/app-landing-page-header/app-landing-page-header.component.html (2)
1-1: LGTM: Background styling fix
The addition of tw-bg-no-repeat class prevents unwanted background image tiling, improving visual consistency.
17-17: LGTM: Text color adjustment
The change from tw-text-text-secondary to tw-text-text-tertiary aligns with the design system's color hierarchy.
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-setup-connection/qbd-direct-setup-connection.component.html (3)
1-1: LGTM: Border styling update
The border color change to tw-border-border-tertiary maintains visual consistency with the design system.
25-26: LGTM: Text wrapping improvements
The addition of tw-max-w-[300px] and tw-break-words classes improves text readability and prevents overflow.
76-78: LGTM: Responsive iframe sizing
The width adjustments using percentage-based values (tw-w-[40%] and tw-w-[100%]) improve responsive behavior.
src/styles.scss (1)
31-33: Verify the SVG icon path and consider using a more specific selector.
The new class uses a hardcoded path to the SVG icon. Additionally, the use of !important suggests potential specificity issues in the CSS cascade.
Consider:
- Using a more specific selector to avoid
!important - Using CSS custom properties for icon paths to make them configurable
-.pi-times.fyle.qbd{
- content: url('./assets/icons/cross.svg') !important;
-}
+:root {
+ --icon-cross: url('./assets/icons/cross.svg');
+}
+
+.pi-times.fyle.qbd {
+ content: var(--icon-cross);
+}✅ Verification successful
The SVG path is correct and the file exists in the codebase
The file cross.svg exists at src/assets/icons/cross.svg, which matches the relative path ./assets/icons/cross.svg used in the CSS. The path resolution will work correctly since styles.scss is in the src directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the SVG icon file exists and is accessible
# Test: Check if the cross.svg file exists in the assets/icons directory
fd -t f "cross.svg" "assets/icons"
Length of output: 132
Script:
#!/bin/bash
# Let's first find all SVG files to understand the assets structure
fd -e svg
# Also search for any references to cross.svg in the codebase
rg -i "cross\.svg"
Length of output: 7234
src/app/integrations/qbd-direct/qbd-direct-shared/qbd-direct-data-sync/qbd-direct-data-sync.component.html (1)
Line range hint 1-39: Overall changes look good
The modifications improve visual consistency and maintain proper component structure. The suggested improvements are minor and can be addressed in follow-up updates.
| @@ -44,7 +44,7 @@ export class QbdDirectOnboardingPreRequisiteComponent { | |||
| { | |||
| id: 1, | |||
| label: 'Install QuickBooks Web Connector', | |||
| caption: 'Download and install the QuickBooks Web Connector on the system where QuickBooks Desktop is installed.', | |||
| caption: `<a href='https://developer.intuit.com/app/developer/qbdesktop/docs/get-started/get-started-with-quickbooks-web-connector' target="_blank" class=" tw-underline !tw-underline-offset-1" >Download</a> and install the QuickBooks Web Connector on the system where QuickBooks Desktop is installed.`, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security attributes to external link
While the addition of the documentation link improves user experience, external links opened in new tabs should include security attributes to prevent potential security vulnerabilities.
- caption: `<a href='https://developer.intuit.com/app/developer/qbdesktop/docs/get-started/get-started-with-quickbooks-web-connector' target="_blank" class=" tw-underline !tw-underline-offset-1" >Download</a> and install the QuickBooks Web Connector on the system where QuickBooks Desktop is installed.`,
+ caption: `<a href='https://developer.intuit.com/app/developer/qbdesktop/docs/get-started/get-started-with-quickbooks-web-connector' target="_blank" rel="noopener noreferrer" class="tw-underline !tw-underline-offset-1">Download</a> and install the QuickBooks Web Connector on the system where QuickBooks Desktop is installed.`,📝 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.
| caption: `<a href='https://developer.intuit.com/app/developer/qbdesktop/docs/get-started/get-started-with-quickbooks-web-connector' target="_blank" class=" tw-underline !tw-underline-offset-1" >Download</a> and install the QuickBooks Web Connector on the system where QuickBooks Desktop is installed.`, | |
| caption: `<a href='https://developer.intuit.com/app/developer/qbdesktop/docs/get-started/get-started-with-quickbooks-web-connector' target="_blank" rel="noopener noreferrer" class="tw-underline !tw-underline-offset-1">Download</a> and install the QuickBooks Web Connector on the system where QuickBooks Desktop is installed.`, |
|
Description
fix: QA fixes after design team testing
Clickup
https://app.clickup.com/t/86cx6efcg
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation