feature]: Add action to import translations per row item#225
feature]: Add action to import translations per row item#225adrianq merged 10 commits intodevelopmentfrom
Conversation
…ck if app is installed in instance
BundleMonNo change in files bundle size Groups updated (1)
Final result: ✅ View report in BundleMon website ➡️ |
deeonwuli
changed the title
|
Task linked: CU-869c9x3n2 There is no way to import translations |
…ImportTranslationDialog
gqcorneby
left a comment
gqcorneby
left a comment
There was a problem hiding this comment.
Thank @deeonwuli! The changes for the import/export looks good and works for me :)
I just have some comments about the refactor regarding installed apps. It's great that you were able to trimming down InstanceRepository and reduced the api calls! 👏 👏 I just have an inline comment about an alternative approach to avoid dependence on InstanceRepository all together when fetching the training modules. Let me know if anything is unclear :)
| return version; | ||
| } | ||
|
|
||
| public async installApp(appName: string): Promise<boolean> { |
There was a problem hiding this comment.
[not tested] Since listInstalledApps is still cached, it might return stale data after installation. This might not have been the case before because we used isAppInstalledByUrl to check installation status. Now that we're using the list to reduce requests, we might need to clear the list cache upon installing an app.
clearCache(this.listInstalledApps, this)
| return isAppInstalled; | ||
| } | ||
|
|
||
| export async function getVersion(api: D2Api): Promise<string> { |
There was a problem hiding this comment.
Should this be memoized this like your implementation in bulk-load? EyeSeeTea/Bulk-Load@65abbfb
| launchUrl: string | ||
| ): Promise<boolean> { | ||
| const isPathRelative = launchUrl.startsWith("/"); | ||
| const [baseAppPath, _] = launchUrl.split("#"); |
There was a problem hiding this comment.
[nitpick] I think the _ can be safely removed
| export interface TrainingModuleRepository extends TranslableTextRepository { | ||
| list(): Promise<TrainingModule[]>; | ||
| get(moduleKey: string, options: GetModuleOptions): Promise<TrainingModule | undefined>; | ||
| list(installedApps: InstalledApp[]): Promise<TrainingModule[]>; |
There was a problem hiding this comment.
passing installed apps to reduce request calls makes sense and is a good improvement but I have a few doubts. With this, you were able to move instanceRepo from the TrainingModuleRepo to the usecase, which I think is cleaner, but it widens this contract.
An alternative is to extract the function that gets the app list to a helper function in the data layer. This helper function can now be used by both the instance repository and the training module repo so fetching the list is not depenent on the instance repo :) I consulted Jorge just to be sure and this was also his suggestion.
… into refactor/fewer-requests-to-check-installed-apps
gqcorneby
left a comment
gqcorneby
left a comment
There was a problem hiding this comment.
Looks good! Thanks @deeonwuli!
4 participants
📌 References
📝 Implementation
installedAppsto check if app is installed thereby reducing api calls📹 Screenshots/Screen capture
Screen.Recording.2026-03-24.at.15.27.30.mov
📑 Others
#869c9x3n2