feat: add weather widget by timowolf · Pull Request #2092 · siemens/element

timowolf · 2026-05-20T21:00:22Z

Documentation.
Examples.
Dashboards Demo.
Playwright report.

Coverage Reports:

gemini-code-assist

Code Review

This pull request introduces a new weather dashboard widget to the library, featuring vertical, horizontal, and compact layouts, an abstract icon resolver, and comprehensive documentation. Feedback identifies a memory leak in the editor component and recommends using signals for derived state. Other suggestions include aligning documentation with implementation regarding forecast column limits, ensuring browser compatibility for CSS Houdini features, and improving internationalization for ARIA labels. Finally, the review notes violations of Siemens UX writing guidelines regarding capitalization and the use of forbidden terms in user-facing messages.

github-actions · 2026-05-21T06:55:37Z

⬇️ Download VRTs

chintankavathia

@timowolf Thanks for the beautiful weather widget. I left some of my findings in comments. additionally can you also check if this is a11y compliant as I had a hard time using voiceover to read through the values.

panch1739

@timowolf I realised that we need to do an adjustment in the values:

The max temperature should be body-lg-bold

I checked with @Danisand and there should be a non-breaking space between the values and the units

timowolf · 2026-05-22T13:03:52Z

@timowolf I realised that we need to do an adjustment in the values:
1. The max temperature should be body-lg-bold 2. I checked with @Danisand and there should be a non-breaking space between the values and the units

good catch - got lost during refactoring

…is disabled The internal loading-state signal in SiWidgetBaseDirective was initialized to `false`, so the computed `showLoadingIndicator() = internal ?? input` short-circuited and never read the `showLoadingIndicator` input. As a result, binding `[showLoadingIndicator]="true"` had no effect when `disableAutoLoadingIndicator` was set (or before the auto-loading timer fired). Initialize the internal signal with `undefined` so the input value takes effect until the directive explicitly overrides it via the auto-loading mechanism or when a value arrives.

Add `SiWeatherWidgetComponent` and `SiWeatherWidgetBodyComponent`, a presentational dashboard widget that visualizes current conditions, daily metrics, and a multi-day forecast in `vertical`, `horizontal`, or `compact` layouts. The widget accepts either a composite `value` or granular `current` / `metrics` / `forecast` inputs (granular overrides composite), renders the location in the card header for non-compact layouts and next to the illustration for `compact`, and exposes an abstract `SiWeatherIconResolver` so consumers can plug in any icon set. A tiered `SiWeatherCondition` model (`Minimal`, `Compact`, `Full`) with `windy` / `freezing` / `rain` / `snow` / `thunder` modifiers describes condition state without locking consumers into a specific provider's vocabulary. The widget ships 115 monochrome SVG illustrations rendered via a CSS mask and themed through `--si-weather-widget-icon-color`, while raster icons resolved by third-party resolvers continue to render as `<img>`. The forecast row supports up to five extra metric columns per day and an automatic loading skeleton via `SiWidgetBaseDirective`. Includes configurable showcase examples, a fully-mocked Weather widget for the flexible dashboards-demo (pre-placed instance and editor wizard), documentation under `docs/components/dashboards/`, three new `SI_WEATHER_WIDGET.*` translatable keys, 30 unit tests, and Playwright VRT specs covering both the showcase examples and the demo flows.

hbxes

Just a few points to discuss, otherwise it's cool and a nice addition to the design system

Co-authored-by: André Fischbach <55880422+hbxes@users.noreply.github.com>

panch1739 · 2026-05-27T06:25:52Z

Just a few points to discuss, otherwise it's cool and a nice addition to the design system

@hbxes thank youuuu, addressed the points :)

panch1739 · 2026-05-27T06:26:53Z

Just a few points to discuss, otherwise it's cool and a nice addition to the design system

@hbxes thank youuuu, addressed the points :)

spliffone · 2026-05-28T16:07:06Z

+/**
+ * Marker directive that types the `let-` context of the shared range
+ * `<ng-template>` so its body gets full type checking instead of `any`.
+ */


This directive should be internal:

Suggested change

*/

@internal

*/

spliffone · 2026-05-28T16:53:05Z

+    }
+    if (this.showForecast()) {
+      const days = aggregateForecast(snap.forecast, 5).map(d => ({
+        label: DAY_LABELS[d.date.getDay()],


We should use the Intl API to get the labels based on the current locale.

spliffone · 2026-05-28T17:06:08Z

+    }
+  }
+
+  private async fetch(city: OpenWeatherCitySuggestion): Promise<void> {


We should use the Angular resource API

spliffone · 2026-05-28T17:15:44Z

+}
+
+// @public
+export interface SiWeatherWidgetCurrent {


How can a consumer distinguish between night and day mode if he wants to change the illustration?

spliffone · 2026-05-28T17:29:39Z

+Applications can override the default by providing their own resolver. A
+resolver returns either an Element icon name (rendered via `<si-icon>`) or a
+direct image URL (rendered as `<img>`); callers can also pass an `src`
+directly via `illustration.src` to bypass the resolver.


What about the CORS settings, should we add a note if the src is in a difference domain or a CDN is used?

spliffone · 2026-05-28T17:36:11Z

+.si-weather-widget-skeleton-value {
+  flex: 0 0 auto;
+  align-self: stretch;
+  inline-size: 5.3125rem; // 85px


we should use ch unit instead

spliffone

Code Review

Good addition to the library. The three-layout design, extensible icon resolver, and auto-loading-indicator integration are well thought through. All CI checks pass. A few items worth addressing before merge:

— regression

The removal of the parameter means every input change (heading, location, accentLine, …) now triggers the loading-indicator logic, not just changes to value. Previously the guard changes.value?.currentValue scoped it correctly. Consider using an effect() keyed on the value signal, or restore the typed SimpleChanges<this> parameter:

ngOnChanges(changes: SimpleChanges<this>): void {
  if (this.disableAutoLoadingIndicator() || !('value' in changes)) return;
  ...
}

JSDoc vs implementation still disagree

The diff for si-weather-widget.types.ts line 71 shows the comment was updated, but the JSDoc on SiWeatherWidgetForecast.columns still reads "up to two optional columns" while the body component and SCSS support five. The timowolf reply says "fixed" — please double-check the final content in the branch.

CSS Houdini () — no browser fallback

si-weather-widget-body.component.scss registers integer custom properties via @property to drive repeat(min(...)) grid tracks. @property is unsupported in Firefox < 128 (released mid-2024) and older Safari. Without a fallback the grid layout silently breaks. Either:

Add a @supports (syntax: "<integer>") guard with a safe fallback grid, or
Replace the CSS Houdini approach with a JS-computed class (e.g., si-weather-widget-cols-3) applied from the component.

— template-driven forms

The project guidelines (and AGENTS.md) require reactive forms. The configurable example uses FormsModule + ngModel. spliffone already suggested a NonNullableFormBuilder approach in the editor component; the same applies here. Since this is example code it is lower priority, but it sets the pattern for consumers.

— inline style

<si-weather-widget style="display: block; inline-size: 500px" ...

Inline styles should be avoided per guidelines. A host class or a wrapper div with a utility class is cleaner.

— redundant API surface

There are two independent alt-text entry points for the illustration: illustration.alt (on SiWeatherIcon) and the top-level illustrationAlt field. The dual API is only needed for the string shorthand case (illustration: 'url'). Consider removing illustrationAlt and requiring callers who need alt text to use the object form { src: '...', alt: '...' }. Keeps the surface smaller and avoids the "which wins?" question.

's open comments from May 28 (most recent)

Several comments posted just yesterday haven't received replies yet:

Forecast layout: use CSS grid instead of per-day classes in si-weather-widget-forecast.component.html
Skeleton component: use ch unit instead of fixed units in si-weather-widget-skeleton.component.scss
Demo editor: use FormBuilder / reactive forms in weather-widget-editor.component.ts
Configurable example: use Intl API for day labels; use Angular resource API for async fetch
Day/night illustration: no way for a consumer to distinguish day vs. night when supplying icons — worth at least a note in the docs or a timeOfDay hint on SiWeatherIcon

These should be addressed or explicitly deferred to a follow-up issue before merging.

Minor nits

si-weather-widget.ts (example): logEvent = inject(LOG_EVENT) is injected but never called in any event binding in the template — dead code unless an output is wired up.
open-weather.service.ts: exporting free functions (mapOpenWeatherCondition, aggregateForecast) alongside the @Injectable class in the same file makes tree-shaking harder. Move to a separate util file or mark as @internal in the barrel.
SiWeatherWidgetForecastColumn.values: readonly (string | number | undefined)[] — allowing undefined in a readonly tuple-like array is fine but consider a JSDoc note explaining what "missing entries" render as, so consumers know it is intentional.

Summary: The core widget implementation is solid and CI is green. The main blockers are the ngOnChanges scope regression, the CSS Houdini fallback gap, and the unresolved spliffone comments from May 28. The rest are improvements for cleanliness but can be deferred.

spliffone

Code Review

Good addition to the library. The three-layout design, extensible icon resolver, and auto-loading-indicator integration are well thought through. All CI checks pass. A few items worth addressing before merge:

`SiWidgetBaseDirective` — `ngOnChanges` scope regression

Removing the SimpleChanges parameter means every input change (heading, location, accentLine, …) now triggers the loading-indicator logic, not just changes to value. Previously changes.value?.currentValue scoped it correctly. Consider using an effect() keyed on the value signal, or restore the typed parameter:

ngOnChanges(changes: SimpleChanges<this>): void {
  if (this.disableAutoLoadingIndicator() || !('value' in changes)) return;
  ...
}

JSDoc vs implementation still disagree on forecast column count

The diff for si-weather-widget.types.ts shows timowolf replied "fixed", but the SiWeatherWidgetForecast.columns JSDoc on the branch still reads "up to two optional columns" while the body component and SCSS support five. Please verify the final state.

CSS Houdini (`@property`) — no browser fallback

si-weather-widget-body.component.scss uses @property to register integer custom properties that drive repeat(min(...)) grid tracks. @property is unsupported in Firefox < 128 and older Safari. Without a fallback the grid layout silently breaks. Options:

Add a @supports (syntax: "<integer>") guard with a safe fallback grid, or
Push column count into a JS-computed class (e.g., si-weather-widget-cols-3) from the component

Configurable example uses template-driven forms

The project guidelines require reactive forms. The configurable example (si-weather-widget-configurable) uses FormsModule + ngModel. spliffone already suggested NonNullableFormBuilder for the demo editor; the same applies here. Lower priority since it is example code, but it sets the pattern for consumers.

Inline style in custom-resolver example

<si-weather-widget style="display: block; inline-size: 500px" ...

Inline styles should be avoided per project guidelines. A host class or wrapper div with a utility class is preferred.

`illustrationAlt` — redundant API surface

SiWeatherWidgetCurrent has both illustration.alt (on SiWeatherIcon) and a top-level illustrationAlt. The dual entry point only makes sense for the string shorthand case. Consider removing illustrationAlt and requiring callers who need alt text to use the object form { src: '...', alt: '...' }. This avoids the "which wins?" ambiguity and keeps the public surface smaller.

`spliffone`'s open comments from May 28 (unaddressed)

Several comments posted yesterday have no replies yet:

Forecast layout: use CSS grid instead of per-day classes in si-weather-widget-forecast.component.html
Skeleton component: use ch units instead of fixed px in si-weather-widget-skeleton.component.scss
Demo editor: use FormBuilder / reactive forms in weather-widget-editor.component.ts
Configurable example: use Intl API for day labels; use Angular resource API for async fetch
Day/night illustration: no way for a consumer to distinguish day vs. night — worth at minimum a doc note or a timeOfDay hint on SiWeatherIcon

These should be addressed or explicitly deferred to a tracked follow-up issue before merging.

Minor nits

si-weather-widget.ts (example): logEvent = inject(LOG_EVENT) is injected but never called in any event binding — dead code unless an output is wired up.
open-weather.service.ts: free functions (mapOpenWeatherCondition, aggregateForecast) exported alongside the @Injectable class in the same file hinder tree-shaking. Move to a separate util file or mark @internal in the barrel.
SiWeatherWidgetForecastColumn.values: readonly (string | number | undefined)[] — a JSDoc note explaining what "missing entries" render as would help consumers.

Summary: Core widget implementation is solid and CI is fully green. Main concerns are the ngOnChanges scope regression in SiWidgetBaseDirective, the missing CSS Houdini fallback, and the batch of unaddressed spliffone comments from May 28. The remaining items are quality improvements that can be deferred if needed.

spliffone

Code Review

Good addition to the library. The three-layout design, extensible icon resolver, and auto-loading-indicator integration are well thought through. All CI checks pass. A few items worth addressing before merge:

`SiWidgetBaseDirective` — `ngOnChanges` scope regression

Removing the SimpleChanges parameter means every input change (heading, location, accentLine, …) now triggers the loading-indicator logic, not just changes to value. Previously changes.value?.currentValue scoped it correctly. Consider using an effect() keyed on the value signal, or restore the typed parameter and guard:

ngOnChanges(changes: SimpleChanges<this>): void {
  if (this.disableAutoLoadingIndicator() || !('value' in changes)) return;
  ...
}

JSDoc vs implementation still disagree on forecast column count

The diff for si-weather-widget.types.ts shows timowolf replied "fixed", but the SiWeatherWidgetForecast.columns JSDoc still reads "up to two optional columns" while the body component and SCSS support five. Please verify the final state on the branch.

CSS Houdini (`@property`) — no browser fallback

si-weather-widget-body.component.scss uses @property to register integer custom properties that drive repeat(min(...)) grid tracks. @property is unsupported in Firefox < 128 and older Safari. Without a fallback the grid layout silently breaks. Options:

Add a @supports (syntax: "<integer>") guard with a safe fallback grid, or
Push column count into a JS-computed class (e.g. si-weather-widget-cols-3) from the component

Configurable example uses template-driven forms

Project guidelines require reactive forms. The configurable example (si-weather-widget-configurable) uses FormsModule + ngModel. spliffone already suggested NonNullableFormBuilder for the demo editor; the same applies here. Lower priority since it is example code, but it sets the wrong pattern for consumers.

Inline style in custom-resolver example

<si-weather-widget style="display: block; inline-size: 500px" ...

Inline styles should be avoided per project guidelines. A host class or wrapper div with a utility class is preferred.

`illustrationAlt` — redundant API surface

SiWeatherWidgetCurrent has both illustration.alt (on SiWeatherIcon) and a top-level illustrationAlt. The dual entry point only makes sense for the string shorthand case. Consider removing illustrationAlt and requiring callers who need alt text to use the object form { src: "...", alt: "..." }. This avoids the "which wins?" ambiguity and keeps the public surface smaller.

`spliffone`'s open comments from May 28 (unaddressed)

Several comments posted yesterday have no replies yet:

Forecast layout: use CSS grid instead of per-day classes in si-weather-widget-forecast.component.html
Skeleton: use ch units instead of fixed px in si-weather-widget-skeleton.component.scss
Demo editor: use FormBuilder / reactive forms in weather-widget-editor.component.ts
Configurable example: use Intl API for day labels; use Angular resource API for async fetch
Day/night illustration: no way for a consumer to distinguish day vs. night — worth at minimum a doc note or a timeOfDay hint on SiWeatherIcon

These should be addressed or explicitly deferred to a tracked follow-up issue before merging.

Minor nits

si-weather-widget.ts (example): logEvent = inject(LOG_EVENT) is injected but never called in any event binding — dead code unless an output is wired up.
open-weather.service.ts: free functions (mapOpenWeatherCondition, aggregateForecast) exported alongside the @Injectable class in the same file hinder tree-shaking. Move to a separate util file or mark @internal in the barrel.
SiWeatherWidgetForecastColumn.values: readonly (string | number | undefined)[] — a JSDoc note explaining what "missing entries" render as would help consumers know this is intentional.

Summary: Core widget is solid and CI is fully green. Main concerns are the ngOnChanges scope regression in SiWidgetBaseDirective, the missing CSS Houdini fallback, and the batch of unaddressed spliffone comments from May 28. Remaining items are quality improvements that can be deferred.

spliffone · 2026-05-29T04:27:53Z

+        id="src-manual"
+        name="src"
+        value="manual"
+        [ngModel]="mode()"


Can we use ReactiveForms since template driven forms require an extra cycle to propagate changes.

spliffone · 2026-05-29T04:28:04Z

+        id="src-ow"
+        name="src"
+        value="openweather"
+        [ngModel]="mode()"


Can we use ReactiveForms since template driven forms?

spliffone · 2026-05-29T04:28:21Z

+            autocomplete="off"
+            placeholder="paste your free API key"
+            name="apiKey"
+            [ngModel]="apiKey()"


Can we use ReactiveForms since template driven forms?

spliffone · 2026-05-29T04:28:40Z

+            id="cfg-location"
+            class="form-control"
+            name="location"
+            [ngModel]="location()"


Can we use ReactiveForms since template driven forms?

spliffone · 2026-05-29T04:28:45Z

+              id="cfg-units"
+              class="form-select"
+              name="units"
+              [ngModel]="units()"


Can we use ReactiveForms since template driven forms?

spliffone · 2026-05-29T04:46:58Z

+
+        @if (mode() === 'openweather') {
+          <div class="col-4">
+            <label class="form-label" for="cfg-units">Units</label>


Typically we prefer to use si-form-item in examples

spliffone · 2026-05-29T04:49:51Z

+  SiWeatherWidgetMetric
+} from '@siemens/element-ng/dashboard';
+
+export const ALL_CONDITIONS: SiWeatherCondition[] = [


What about snow or rain + snow?

spliffone · 2026-05-29T04:52:52Z

+ * provider-specific vocabularies (Xweather, OpenWeather, …) should register
+ * their own resolver and may accept any string in their public APIs.
+ */
+export type SiWeatherCondition = 'clear' | 'clouds' | 'rain' | 'storm' | 'wind' | 'unknown';


What about snow, drizzle, fog or mixed conditions?

spliffone · 2026-05-29T04:58:26Z

+ *
+ * - `vertical` (default): full vertical card with `<si-card>` heading,
+ *   illustration, today block, optional additional data and an optional
+ *   forecast table with up to two extra columns.


Do you mean five extra columns?

spliffone · 2026-05-29T04:58:51Z

+/**
+ * Forecast block of the weather widget.
+ *
+ * The vertical layout supports up to two optional {@link columns} in addition


Do you mean up to five columns?

spliffone · 2026-05-29T05:03:08Z

+];
+
+const API_KEY_STORAGE = 'si-weather-widget-demo.openweather-api-key';
+const DAY_LABELS = ['Sun', 'Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat'];


Day labels should be retrieved via Intl API

spliffone · 2026-05-29T05:38:29Z

@@ -0,0 +1,287 @@
+<div class="row align-items-start">
+  <aside class="col-lg-4 col-md-5 mb-6">
+    <h5>Data source</h5>


Usually we use the Control panel approach which is hidden during e2e tests

timowolf added the feature Marks feature requests and feature implementations label May 20, 2026

timowolf added this to the 49.x milestone May 20, 2026

github-advanced-security AI found potential problems May 20, 2026

View reviewed changes

Comment thread projects/element-ng/dashboard/widgets/si-weather-widget/si-weather-widget-body.component.ts Fixed

gemini-code-assist Bot reviewed May 20, 2026

View reviewed changes

timowolf force-pushed the feat/weather-widget-component branch from b1e8ffc to 07a9333 Compare May 20, 2026 21:15

timowolf requested a review from chintankavathia May 21, 2026 07:04

timowolf force-pushed the feat/weather-widget-component branch 4 times, most recently from 51530b6 to 6a8991c Compare May 21, 2026 13:25

chintankavathia requested changes May 21, 2026

View reviewed changes

timowolf force-pushed the feat/weather-widget-component branch 8 times, most recently from ed7e2b7 to 3e570df Compare May 22, 2026 11:53

timowolf marked this pull request as ready for review May 22, 2026 11:55

timowolf requested review from a team and spliffone as code owners May 22, 2026 11:55

chintankavathia requested changes May 22, 2026

View reviewed changes

panch1739 reviewed May 22, 2026

View reviewed changes

timowolf force-pushed the feat/weather-widget-component branch from 3e570df to 80788a9 Compare May 22, 2026 13:03

timowolf force-pushed the feat/weather-widget-component branch 2 times, most recently from 4bbabe5 to a168cdd Compare May 22, 2026 13:29

timowolf and others added 3 commits May 26, 2026 11:24

feat(weather-widget): add ux docs

f2d893d

chintankavathia force-pushed the feat/weather-widget-component branch from 26080ba to f2d893d Compare May 26, 2026 05:54

chintankavathia enabled auto-merge May 26, 2026 05:55

hbxes reviewed May 26, 2026

View reviewed changes

docs(weather-widget): correct ux writing

b5432ff

Co-authored-by: André Fischbach <55880422+hbxes@users.noreply.github.com>

spliffone reviewed May 28, 2026

View reviewed changes

spliffone reviewed May 29, 2026

View reviewed changes

Conversation

timowolf commented May 20, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chintankavathia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

panch1739 left a comment

Choose a reason for hiding this comment

Uh oh!

timowolf commented May 22, 2026

Uh oh!

hbxes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

panch1739 commented May 27, 2026

Uh oh!

panch1739 commented May 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spliffone left a comment

Choose a reason for hiding this comment

Code Review

— regression

JSDoc vs implementation still disagree

CSS Houdini () — no browser fallback

— template-driven forms

— inline style

— redundant API surface

's open comments from May 28 (most recent)

Minor nits

Uh oh!

timowolf commented May 20, 2026 •

edited by github-actions Bot

Loading

github-actions Bot commented May 21, 2026 •

edited

Loading

`SiWidgetBaseDirective` — `ngOnChanges` scope regression

CSS Houdini (`@property`) — no browser fallback

`illustrationAlt` — redundant API surface

`spliffone`'s open comments from May 28 (unaddressed)

`SiWidgetBaseDirective` — `ngOnChanges` scope regression

CSS Houdini (`@property`) — no browser fallback

`illustrationAlt` — redundant API surface

`spliffone`'s open comments from May 28 (unaddressed)

spliffone May 29, 2026 •

edited

Loading