Merge new starter#168
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Siemens Industrial Experience starter applications (Angular, React, and Vue) to version 5.0.0, introducing unified feature pages (Get Started, Forms, Charts, and Grids) and setting up Playwright for E2E testing. The code review highlights several critical improvements: using CSS display properties instead of conditional rendering to preserve ResizeObserver instances on chart containers, fixing a memory leak in the React chart cleanup by correctly referencing the active echarts instance, ensuring type safety in form components when handling null or undefined event details, and adopting Angular's idiomatic routerLinkActive directive to simplify active route management.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| @if (showTable) { | ||
| <section class="table-container" aria-label="Chart data table"> | ||
| <ag-grid-angular | ||
| [rowData]="chartTableData" | ||
| [columnDefs]="chartTableColDefs" | ||
| [suppressMovableColumns]="true" | ||
| [domLayout]="'autoHeight'" | ||
| [rowHeight]="36" | ||
| [theme]="ixTheme" | ||
| aria-label="Motor vibration data table" | ||
| /> | ||
| </section> | ||
| } @else { | ||
| <figure | ||
| #chartContainer | ||
| aria-labelledby="chart-title" | ||
| class="chart-container" | ||
| > | ||
| <figcaption class="sr-only">Bar chart showing monthly production output</figcaption> | ||
| </figure> | ||
| } |
There was a problem hiding this comment.
Using @if/@else destroys and recreates the DOM elements. When the <figure> element is destroyed, the ResizeObserver (which was registered on the initial element) stops working and does not automatically observe the newly created <figure> element when toggled back. Toggling visibility via [style.display] keeps the elements in the DOM, preserving the ResizeObserver and avoiding unnecessary recreation of the chart and grid instances.
<section [style.display]="showTable ? 'block' : 'none'" class="table-container" aria-label="Chart data table">
<ag-grid-angular
[rowData]="chartTableData"
[columnDefs]="chartTableColDefs"
[suppressMovableColumns]="true"
[domLayout]="'autoHeight'"
[rowHeight]="36"
[theme]="ixTheme"
aria-label="Motor vibration data table"
/>
</section>
<figure
#chartContainer
[style.display]="showTable ? 'none' : 'block'"
aria-labelledby="chart-title"
class="chart-container"
>
<figcaption class="sr-only">Bar chart showing monthly production output</figcaption>
</figure>| {showTable ? ( | ||
| <section className={styles.tableContainer} aria-label="Chart data table"> | ||
| <AgGridReact | ||
| rowData={CHART_TABLE_DATA} | ||
| columnDefs={CHART_TABLE_COL_DEFS} | ||
| suppressMovableColumns={true} | ||
| domLayout="autoHeight" | ||
| rowHeight={36} | ||
| theme={ixTheme} | ||
| aria-label="Motor vibration data table" | ||
| /> | ||
| </section> | ||
| ) : ( | ||
| <figure | ||
| ref={chartRef} | ||
| aria-labelledby="chart-title" | ||
| className={styles.chartContainer} | ||
| > | ||
| <figcaption className="sr-only">Bar chart showing monthly production output</figcaption> | ||
| </figure> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
Conditional rendering destroys and recreates the DOM elements. When the <figure> element is unmounted, the ResizeObserver (which was registered on the initial element) stops working and does not observe the newly mounted <figure> element when toggled back. Toggling visibility via CSS display properties keeps the elements in the DOM, preserving the ResizeObserver and avoiding unnecessary recreation of the chart and grid instances.
<section style={{ display: showTable ? 'block' : 'none' }} className={styles.tableContainer} aria-label="Chart data table">
<AgGridReact
rowData={CHART_TABLE_DATA}
columnDefs={CHART_TABLE_COL_DEFS}
suppressMovableColumns={true}
domLayout="autoHeight"
rowHeight={36}
theme={ixTheme}
aria-label="Motor vibration data table"
/>
</section>
<figure
ref={chartRef}
style={{ display: showTable ? 'none' : 'block' }}
aria-labelledby="chart-title"
className={styles.chartContainer}
>
<figcaption className="sr-only">Bar chart showing monthly production output</figcaption>
</figure>
|
|
||
| themeSwitcher.themeChanged.on(handleThemeChange); | ||
|
|
||
| const instance = instanceRef.current; | ||
| return () => { | ||
| observer.disconnect(); |
There was a problem hiding this comment.
The instance variable captures instanceRef.current at the time the useEffect setup function runs (which is on mount, when it is null). Consequently, instance remains null inside the cleanup function, and the actual active echarts instance is never disposed, causing a memory leak. Accessing instanceRef.current directly inside the cleanup function resolves this issue.
return () => {
observer.disconnect();
themeSwitcher.themeChanged.off(handleThemeChange);
instanceRef.current?.dispose();
};
| @value-change="inspectionType = String($event.detail)" | ||
| > | ||
| <IxSelectItem v-for="type in INSPECTION_TYPES" :key="type" :value="type" :label="type" /> | ||
| </IxSelect> |
There was a problem hiding this comment.
Using String($event.detail) is unsafe because if the selection is cleared (emitting null or undefined), it will literally convert them to the strings "null" or "undefined". Adding a fallback ensures correct behavior.
@value-change="inspectionType = $event.detail ? String($event.detail) : ''"
| toggleView() { | ||
| this.showTable = !this.showTable; | ||
| if (!this.showTable) { | ||
| setTimeout(() => { | ||
| if (this.chartContainer?.nativeElement) { | ||
| const existingInstance = echarts.getInstanceByDom(this.chartContainer.nativeElement); | ||
| if (existingInstance) { | ||
| existingInstance.dispose(); | ||
| } | ||
| this.chartInstance = echarts.init(this.chartContainer.nativeElement, getEChartsThemeName()); | ||
| this.chartInstance.setOption(buildChartOptions()); | ||
| } | ||
| }, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
With the visibility of the chart now toggled via CSS display properties, there is no need to manually recreate or dispose of the chart instance inside toggleView(). This simplifies the method and avoids potential race conditions.
toggleView() {
this.showTable = !this.showTable;
}| <ix-menu enableToggleTheme aria-label="Main navigation"> | ||
| <a routerLink="" tabindex="-1"> | ||
| <ix-menu-item icon="home" [attr.active]="isActiveRoute('/') ? '' : null"> | ||
| Get Started | ||
| </ix-menu-item> | ||
| </a> | ||
| <a [routerLink]="'devices'"> | ||
| <ix-menu-item | ||
| icon="project" | ||
| [class.active]="activePage === 'devices'" | ||
| (click)="activePage = 'devices'" | ||
| > | ||
| {{ "devices" | translate }} | ||
| <a routerLink="forms" tabindex="-1"> | ||
| <ix-menu-item icon="text-document" [attr.active]="isActiveRoute('/forms') ? '' : null"> | ||
| Forms | ||
| </ix-menu-item> | ||
| </a> | ||
| <a routerLink="charts" tabindex="-1"> | ||
| <ix-menu-item icon="piechart" [attr.active]="isActiveRoute('/charts') ? '' : null"> | ||
| Charts | ||
| </ix-menu-item> | ||
| </a> | ||
| <a routerLink="grids" tabindex="-1"> | ||
| <ix-menu-item icon="table" [attr.active]="isActiveRoute('/grids') ? '' : null"> | ||
| Grids | ||
| </ix-menu-item> | ||
| </a> | ||
|
|
||
| <ix-menu-settings | ||
| (tabChange)="setActiveSettingsTab($event)" | ||
| label="{{ 'settings.title' | translate }}" | ||
| > | ||
| <ix-menu-settings-item label="{{ 'settings.user-settings' | translate }}"> | ||
| @if (activeSettingsTab === "user-settings") { | ||
| <app-settings /> | ||
| } | ||
| </ix-menu-settings-item> | ||
| <ix-menu-settings-item | ||
| label="{{ 'settings.application-settings' | translate }}" | ||
| > | ||
| @if (activeSettingsTab === "application-settings") { | ||
| <section class="application-settings"> | ||
| <section> | ||
| <ix-typography format="h4" class="headline"> | ||
| {{ "settings.notification" | translate }} | ||
| </ix-typography> | ||
| <ix-toggle textOn="On" textOff="Off"> </ix-toggle> | ||
| </section> | ||
| <section> | ||
| <ix-typography format="h4" class="headline"> | ||
| {{ "settings.import-device" | translate }} | ||
| </ix-typography> | ||
| <ix-upload | ||
| class="upload" | ||
| i18nUploadFile="{{ 'settings.upload-button' | translate }}" | ||
| selectFileText="{{ 'settings.upload-text' | translate }}" | ||
| ></ix-upload> | ||
| </section> | ||
| </section> | ||
| } | ||
| </ix-menu-settings-item> | ||
| </ix-menu-settings> | ||
| </ix-menu> |
There was a problem hiding this comment.
Using routerLinkActive with a template reference variable is the standard, idiomatic Angular way to manage active links. It is robust against query parameters/fragments and allows using standard property binding [active] instead of [attr.active].
<ix-menu enableToggleTheme aria-label="Main navigation">
<a routerLink="" routerLinkActive #rlaHome="routerLinkActive" [routerLinkActiveOptions]="{exact: true}" tabindex="-1">
<ix-menu-item icon="home" [active]="rlaHome.isActive">
Get Started
</ix-menu-item>
</a>
<a routerLink="forms" routerLinkActive #rlaForms="routerLinkActive" tabindex="-1">
<ix-menu-item icon="text-document" [active]="rlaForms.isActive">
Forms
</ix-menu-item>
</a>
<a routerLink="charts" routerLinkActive #rlaCharts="routerLinkActive" tabindex="-1">
<ix-menu-item icon="piechart" [active]="rlaCharts.isActive">
Charts
</ix-menu-item>
</a>
<a routerLink="grids" routerLinkActive #rlaGrids="routerLinkActive" tabindex="-1">
<ix-menu-item icon="table" [active]="rlaGrids.isActive">
Grids
</ix-menu-item>
</a>
</ix-menu>
| RouterOutlet, | ||
| } from '@angular/router'; | ||
| import { Component, ErrorHandler, OnInit, ViewChild } from '@angular/core'; | ||
| import { RouterLink, RouterOutlet, Router } from '@angular/router'; |
There was a problem hiding this comment.
Import RouterLinkActive to support the idiomatic active link management in the template.
| import { RouterLink, RouterOutlet, Router } from '@angular/router'; | |
| import { RouterLink, RouterOutlet, Router, RouterLinkActive } from '@angular/router'; |
| imports: [ | ||
| CommonModule, | ||
| RouterOutlet, | ||
| RouterLink, | ||
| IxApplication, | ||
| IxApplicationHeader, | ||
| IxAvatar, | ||
| IxMenu, | ||
| IxMenuItem, | ||
| IxAvatar, | ||
| IxDropdownItem, | ||
| IxMenuSettingsItem, | ||
| IxMenuSettings, | ||
| RouterOutlet, | ||
| RouterLink, | ||
| IxTypography, | ||
| IxToggle, | ||
| IxUpload, | ||
| SettingsComponent, | ||
| TranslateModule, | ||
| IxContent, | ||
| ErrorBoundaryComponent, | ||
| ], |
There was a problem hiding this comment.
Add RouterLinkActive to the component's imports array.
| imports: [ | |
| CommonModule, | |
| RouterOutlet, | |
| RouterLink, | |
| IxApplication, | |
| IxApplicationHeader, | |
| IxAvatar, | |
| IxMenu, | |
| IxMenuItem, | |
| IxAvatar, | |
| IxDropdownItem, | |
| IxMenuSettingsItem, | |
| IxMenuSettings, | |
| RouterOutlet, | |
| RouterLink, | |
| IxTypography, | |
| IxToggle, | |
| IxUpload, | |
| SettingsComponent, | |
| TranslateModule, | |
| IxContent, | |
| ErrorBoundaryComponent, | |
| ], | |
| imports: [ | |
| CommonModule, | |
| RouterOutlet, | |
| RouterLink, | |
| RouterLinkActive, | |
| IxApplication, | |
| IxApplicationHeader, | |
| IxAvatar, | |
| IxMenu, | |
| IxMenuItem, | |
| IxContent, | |
| ErrorBoundaryComponent, | |
| ], |
| constructor(private router: Router, private errorHandler: ErrorHandler) {} | ||
|
|
||
| private setActivePageFromUrl(url: string) { | ||
| if (url.includes('devices')) { | ||
| this.activePage = 'devices'; | ||
| } else if (url.includes('overview')) { | ||
| this.activePage = 'overview'; | ||
| } | ||
| } | ||
|
|
||
| openModal() { | ||
| useShowDemoMessage(); | ||
| ngOnInit() { | ||
| (this.errorHandler as GlobalErrorHandler).registerBoundary(this.boundary); | ||
| } | ||
|
|
||
| setActiveSettingsTab(event: any) { | ||
| const selectedLabel = event.detail; | ||
|
|
||
| const userSettingsLabel = this.translateService.instant('settings.user-settings'); | ||
| const appSettingsLabel = this.translateService.instant('settings.application-settings'); | ||
|
|
||
| if (selectedLabel === userSettingsLabel) { | ||
| this.activeSettingsTab = 'user-settings'; | ||
| } else if (selectedLabel === appSettingsLabel) { | ||
| this.activeSettingsTab = 'application-settings'; | ||
| isActiveRoute(path: string): boolean { | ||
| if (path === '/') { | ||
| return this.router.url === '/' || this.router.url === ''; | ||
| } | ||
| return this.router.url === path || this.router.url.startsWith(path + '/'); | ||
| } |
There was a problem hiding this comment.
With routerLinkActive handling the active states in the template, the Router injection and the custom isActiveRoute helper method can be completely removed. Additionally, the cast to GlobalErrorHandler is unsafe if ErrorHandler is decorated or replaced at runtime; checking if registerBoundary exists on the handler is a safer defensive programming practice.
constructor(private errorHandler: ErrorHandler) {}
ngOnInit() {
if (this.errorHandler && 'registerBoundary' in this.errorHandler) {
(this.errorHandler as any).registerBoundary(this.boundary);
}
}| onInspectionTypeChange(event: CustomEvent<string | string[]>) { | ||
| const value = event.detail; | ||
| this.inspectionType = Array.isArray(value) ? value[0] || '' : value; | ||
| } |
There was a problem hiding this comment.
If event.detail is null or undefined, Array.isArray(value) is false, and this.inspectionType will be set to undefined instead of a string. Adding a fallback ensures type safety.
| onInspectionTypeChange(event: CustomEvent<string | string[]>) { | |
| const value = event.detail; | |
| this.inspectionType = Array.isArray(value) ? value[0] || '' : value; | |
| } | |
| onInspectionTypeChange(event: CustomEvent<string | string[]>) { | |
| const value = event.detail; | |
| this.inspectionType = Array.isArray(value) ? value[0] || '' : value || ''; | |
| } |
danielleroux
merged commit eece55b
into
siemens:feat/new-starter-apps
* Merge new starter (#168) * cleanup old apps * remove lock files * Update readme * add vitest to react * update dependencies add better tsconfig support * remove playwright * add example test * replace with vitest * fix typings and versions * add oxe lint for react * add stencil patch * fix css module * fix error lint * Revert "fix error lint" This reverts commit d75d996. * Revert "add oxe lint for react" This reverts commit a0ac394. * cleanup * add shadow copy of angular new project * update angular dependencies * remove unused angular stuff * remove old readme * fix angular build * add vitest browser for angular * angular tests * Update readme oss * fix icon usage * cleanup deployment and test execution * add deployment * fix test exec * install playwright test as dependency * remove error boundary * add more options * test new expect * test force click * add wrapper * add vue wrapper * remove pre-icon loading * fix logo issue * fix company issue * Replace type patch with type file to resolve conflict * fix tabindex on content --------- Co-authored-by: Devdarsh Panth <156164395+1307-Dev@users.noreply.github.com>
2 participants
Added the new starter apps after renaming older ones with a legacy prefix. Have updated the README while adhering to the previous format.