feat(startup): implement smart component and integrate data loading#599
feat(startup): implement smart component and integrate data loading#599kyasbal wants to merge 11 commits intoGoogleCloudPlatform:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the startup dialog into a smart-dumb architecture, replaces the angular-split layout with GoldenLayout for the main application view, and updates project coding rules. Feedback includes correcting a CSS typo in the documentation, preventing double event emissions during title changes, and reversing the inspection list sort order to prioritize recent items. Additionally, the reviewer recommended implementing error handling for several API subscriptions to improve UI consistency and user feedback.
| - Responsible for state management and data fetching. | ||
| - Allowed to depend on Angular Services. | ||
| - Smart component never have its layout. It must just embed a single dumb component. If you need layouting multiple dumb component, define a foo-layout.component in the components folder just for layout. | ||
| - Smart component's `:host` selector must have `display: content;` not to affect styling calculation. This is allowed to be in `@Component` definition directly. |
There was a problem hiding this comment.
There is a typo in the CSS property name. It should be display: contents; (with an 's') instead of display: content;. The contents value makes the container disappear, making its children direct children of the next level up in the DOM, which is the intended behavior for smart components in this architecture.
| - Smart component's `:host` selector must have `display: content;` not to affect styling calculation. This is allowed to be in `@Component` definition directly. | |
| - Smart component's :host selector must have display: contents; not to affect styling calculation. This is allowed to be in @Component definition directly. |
| } | ||
|
|
||
| /** Commits the title change and exits editing mode. */ | ||
| protected commitTitleChange(): void { |
There was a problem hiding this comment.
The commitTitleChange method is triggered by both blur and keydown.enter events on the input element. When the Enter key is pressed, isEditing is set to false, which causes Angular to remove the input from the DOM. This removal often triggers a blur event, leading to a second execution of this method. Since the parent component might not have updated the item input yet, the second call will see that the taskNameInput still differs from item().label and emit the output again. Adding a guard to check isEditing() prevents this double emission.
protected commitTitleChange(): void {
if (!this.isEditing()) return;
this.isEditing.set(false);| const keys = Object.keys(tp.inspections).sort( | ||
| (a, b) => | ||
| tp.inspections[a].header.inspectTimeUnixSeconds - | ||
| tp.inspections[b].header.inspectTimeUnixSeconds, | ||
| ); |
There was a problem hiding this comment.
The current sorting logic for the inspection list is ascending (a - b), which places the oldest inspections at the top. For a history or activity list, it is generally more user-friendly to show the most recent items first (descending order, b - a).
| const keys = Object.keys(tp.inspections).sort( | |
| (a, b) => | |
| tp.inspections[a].header.inspectTimeUnixSeconds - | |
| tp.inspections[b].header.inspectTimeUnixSeconds, | |
| ); | |
| const keys = Object.keys(tp.inspections).sort( | |
| (a, b) => | |
| tp.inspections[b].header.inspectTimeUnixSeconds - | |
| tp.inspections[a].header.inspectTimeUnixSeconds, | |
| ); |
| this.backendAPI | ||
| .patchInspection(event.id, { name: event.changeTo }) | ||
| .subscribe(() => { | ||
| console.log(`inspection title for task ${event.id} was updated`); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The API calls (e.g., patchInspection, cancelInspection, getInspectionMetadata) are missing error handling in their subscriptions. If a request fails, the user receives no feedback, and the UI state (like the closed edit mode) might become inconsistent with the backend. It is recommended to add an error callback to the subscribe calls, perhaps using a notification service to inform the user.
9ddbcc0 to
1796985
Compare
- Removed `KHIIconRegistrationModule` and `...environment.pluginModules` from `imports` in `web/src/app/root.component.ts`. - Removed imports of `KHIIconRegistrationModule` and `environment` from `web/src/app/root.component.ts`. - Created `web/src/main.spec.ts` with a test for `appConfig` initialization. - In `web/src/main.ts`, moved the configuration object from `bootstrapApplication` to a new exported `appConfig` constant. - Added `importProvidersFrom(KHIIconRegistrationModule)` and `importProvidersFrom(...environment.pluginModules)` to `appConfig.providers` in `web/src/main.ts`.
サイドメニュー(ロゴ、タイトル、イタリック体のキャッチフレーズ、ボタン類)の完成形。
インスペクションリストとリストアイテムの完成形。
ダイアログ全体のレイアウトとコンテンツエリアの完成形。
インスペクションリストとリストアイテムの完成形。
データ取得、Smartコンポーネントの実装と、既存の呼び出し元との統合。
1796985 to
bedd962
Compare
Motivation
To finalize the startup dialog migration by adding the smart component for state management and data fetching, and replacing the legacy startup component. This PR depends on the layout completed in PR #598.
Changes
StartupSmartComponentto handle data loading and state management.動機
状態管理とデータ取得のためのSmartコンポーネントを追加し、レガシーなスタートアップコンポーネントを置き換えることで、スタートアップダイアログの移行を完了させます。このPRは、PR #598 で完成したレイアウトに依存しています。
変更内容
StartupSmartComponentを追加しました。