feat/Enable overriding Flatfile default loading component #216
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant AngularApp
participant SpaceComponent
Developer->>AngularApp: Set loading property
AngularApp->>SpaceComponent: Initialize component with loading state
SpaceComponent->>SpaceComponent: ngOnChanges() triggered on input change
SpaceComponent->>ChangeDetectorRef: Trigger change detection
SpaceComponent->>AngularApp: Render loading or content based on loading state
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
🧹 Outside diff range and nitpick comments (4)
.changeset/proud-books-rush.md (2)
1-3: Consider using full package name instead of 'angular'.While the minor version bump is appropriate for this feature addition, consider using the full package name '@flatfile/angular-sdk' instead of just 'angular' for better clarity.
--- -'angular': minor +'@flatfile/angular-sdk': minor ---
5-5: Enhance the changeset description with implementation details.The current description could be more informative by including key implementation details.
-Allow overriding the default loading component in Angular through the loading property. +Allow overriding the default loading component in Angular by assigning a custom TemplateRef to the ISpace['loading'] property. This enables users to replace the default loading spinner with their own loading template.packages/angular/angular-sdk/src/lib/sdk/space.component.ts (2)
114-114: Consider removing redundant change detection call.The initial
markForCheck()call at the start ofinitSpacemight be redundant since the method is already triggered by eitherngOnInitor the service subscription, both of which run within Angular's change detection cycle.initSpace = async (spaceProps: ReusedOrOnboarding) => { - this.changeDetectorRef.markForCheck() this.closeInstance = false // ... rest of the method } finally { this.changeDetectorRef.markForCheck() } }Also applies to: 152-153
Line range hint
95-103: Consider making the API URL configurable through environment variables.The hardcoded API URL
'https://platform.flatfile.com/api'appears in multiple places in the code. This could make it difficult to switch environments or handle different deployment scenarios.Consider:
- Moving this URL to an environment configuration file
- Providing it through Angular's dependency injection system
- Creating a configuration service to manage such values
Example implementation:
// config.service.ts @Injectable({providedIn: 'root'}) export class ConfigService { readonly apiUrl = environment.flatfileApiUrl; } // environment.ts export const environment = { flatfileApiUrl: 'https://platform.flatfile.com/api' };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.changeset/proud-books-rush.md(1 hunks)packages/angular/angular-sdk/src/lib/sdk/space.component.html(1 hunks)packages/angular/angular-sdk/src/lib/sdk/space.component.ts(6 hunks)
🔇 Additional comments (2)
packages/angular/angular-sdk/src/lib/sdk/space.component.ts (2)
Line range hint 1-29: LGTM! Clean implementation of change detection imports.
The addition of ChangeDetectorRef, OnChanges, and SimpleChanges imports along with the OnChanges interface implementation follows Angular best practices for handling component changes.
40-43: LGTM! Proper dependency injection.
The ChangeDetectorRef injection follows Angular's dependency injection best practices with appropriate access modifiers.
Please explain how to summarize this PR for the Changelog:
Right now, with the @flatfile/angular-sdk, it is not possible to replace the default loading spinner in Flatfile. With this PR, a user can assign a
TemplateRefto theISpace['loading']property and it will correctly show the new loading.Tell code reviewer how and what to test:
In your HTML, add a Template like so:
=> app.component.html
Then in your component, assign the TemplateRef as such:
=> app.component.ts
custom-loading.mp4