feat(weather): redesigned weather app with TypeScript and web components#670
feat(weather): redesigned weather app with TypeScript and web components#670nicomiguelino wants to merge 2 commits intomasterfrom
Conversation
… web components - Modernized implementation using TypeScript, edge-apps-library utilities, and web components - Includes current weather display and 8-item hourly forecast with glassmorphic design Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a redesigned “weather-new” Edge App implemented in TypeScript, using @screenly/edge-apps utilities and web components to render current conditions and an 8-item forecast with updated styling and a new icon set.
Changes:
- Added TypeScript modules for fetching current weather, hourly forecast, and reverse-geocoded location using OpenWeatherMap.
- Added a full set of SVG weather icons and a key→asset mapping used by the weather logic.
- Added a new UI (HTML + CSS) and app manifests/scripts to build and run the app with Bun tooling.
Reviewed changes
Copilot reviewed 12 out of 35 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| edge-apps/weather-new/static/images/icons/windy.svg | Adds new windy icon asset. |
| edge-apps/weather-new/static/images/icons/thunderstorm.svg | Adds new thunderstorm icon asset. |
| edge-apps/weather-new/static/images/icons/thunderstorm-night.svg | Adds new thunderstorm-night icon asset. |
| edge-apps/weather-new/static/images/icons/snow.svg | Adds new snow icon asset. |
| edge-apps/weather-new/static/images/icons/sleet.svg | Adds new sleet icon asset. |
| edge-apps/weather-new/static/images/icons/sleet-night.svg | Adds new sleet-night icon asset. |
| edge-apps/weather-new/static/images/icons/rainy.svg | Adds new rainy icon asset. |
| edge-apps/weather-new/static/images/icons/rain-night.svg | Adds new rain-night icon asset. |
| edge-apps/weather-new/static/images/icons/partlysunny.svg | Adds new partly-sunny icon asset. |
| edge-apps/weather-new/static/images/icons/partially-cloudy.svg | Adds new partially-cloudy icon asset. |
| edge-apps/weather-new/static/images/icons/partially-cloudy-night.svg | Adds new partially-cloudy-night icon asset. |
| edge-apps/weather-new/static/images/icons/mostly-cloudy.svg | Adds new mostly-cloudy icon asset. |
| edge-apps/weather-new/static/images/icons/mostly-cloudy-night.svg | Adds new mostly-cloudy-night icon asset. |
| edge-apps/weather-new/static/images/icons/haze.svg | Adds new haze icon asset. |
| edge-apps/weather-new/static/images/icons/fog.svg | Adds new fog icon asset. |
| edge-apps/weather-new/static/images/icons/fewdrops.svg | Adds new “few drops” icon asset. |
| edge-apps/weather-new/static/images/icons/drizzle.svg | Adds new drizzle icon asset. |
| edge-apps/weather-new/static/images/icons/cloudy.svg | Adds new cloudy icon asset. |
| edge-apps/weather-new/static/images/icons/clear.svg | Adds new clear-day icon asset. |
| edge-apps/weather-new/static/images/icons/clear-night.svg | Adds new clear-night icon asset. |
| edge-apps/weather-new/static/images/icons/chancesleet.svg | Adds new chance-sleet icon asset. |
| edge-apps/weather-new/src/weather.ts | Implements current weather + forecast fetching/parsing and icon selection. |
| edge-apps/weather-new/src/weather-icons.ts | Maps icon keys to bundled SVG assets. |
| edge-apps/weather-new/src/settings.ts | Adds measurement-unit setting accessor. |
| edge-apps/weather-new/src/main.ts | App bootstrap: reads metadata/locale/timezone, renders UI, refresh loop. |
| edge-apps/weather-new/src/location.ts | Reverse-geocodes coordinates to city name via OpenWeatherMap. |
| edge-apps/weather-new/src/css/style.css | Adds glassmorphic layout/styling for current weather + forecast card. |
| edge-apps/weather-new/screenly_qc.yml | Adds QC manifest/settings for the new app. |
| edge-apps/weather-new/screenly.yml | Adds production manifest/settings for the new app. |
| edge-apps/weather-new/package.json | Adds Bun/build/lint/typecheck scripts and workspace dependency wiring. |
| edge-apps/weather-new/index.html | Adds app HTML shell with Screenly components and dist asset references. |
| edge-apps/weather-new/bun.lock | Adds lockfile for the new app’s dependencies. |
| edge-apps/weather-new/README.md | Documents settings and dev/build/deploy workflows. |
| edge-apps/weather-new/.gitignore | Ignores build artifacts and dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'partially-cloudy-night': partiallyCloudyNightIcon, | ||
| partlysunny: partlySunnyIcon, | ||
| rainy: rainyIcon, | ||
| 'rain-night': rainNightIcon, | ||
| sleet: sleetIcon, | ||
| 'sleet-night': sleetNightIcon, |
There was a problem hiding this comment.
getWeatherIconKey() from @screenly/edge-apps returns the key rain for IDs 500–599, but this map defines rainy instead. As a result, rainy conditions will fall back to clear and display the wrong icon. Add a rain entry (and ensure keys match all possible getWeatherIconKey outputs) or rename the key to rain.
| cod?: number | string | ||
| main?: { temp?: number } | ||
| }): boolean { | ||
| return (data.cod === 200 || data.cod === '200') && Boolean(data.main?.temp) |
There was a problem hiding this comment.
isValidWeatherResponse() uses Boolean(data.main?.temp), which rejects valid temperatures of 0 (e.g., 0°C). This will cause the app to treat a valid API response as invalid and show no weather. Prefer checking data.main?.temp !== undefined (and/or Number.isFinite) instead of truthiness.
| return (data.cod === 200 || data.cod === '200') && Boolean(data.main?.temp) | |
| const hasValidCode = data.cod === 200 || data.cod === '200' | |
| const temp = data.main?.temp | |
| const hasValidTemp = typeof temp === 'number' && Number.isFinite(temp) | |
| return hasValidCode && hasValidTemp |
| const temperature = Math.round(data.main.temp) | ||
| const tempHigh = Math.round(data.main.temp_max) | ||
| const tempLow = Math.round(data.main.temp_min) | ||
| const weatherId = data.weather?.[0]?.id ?? null |
There was a problem hiding this comment.
getCurrentWeather() rounds data.main.temp_max/temp_min without validating they exist. If the API response is missing these fields (or they are non-numeric), Math.round will produce NaN and the UI will render NaN°. Consider extending response validation to include temp_min/temp_max (or defaulting them to temperature).
|
|
||
| const data = await response.json() | ||
|
|
||
| if (data.cod !== '200' || !Array.isArray(data.list)) { |
There was a problem hiding this comment.
getHourlyForecast() only accepts data.cod === '200', but earlier you note cod can be a number or string. If this endpoint ever returns 200 as a number, the function will incorrectly return an empty forecast. Consider accepting both 200 and '200' for consistency.
| if (data.cod !== '200' || !Array.isArray(data.list)) { | |
| if ((data.cod !== '200' && data.cod !== 200) || !Array.isArray(data.list)) { |
| forecastItemsEl.innerHTML = items | ||
| .map( | ||
| (item) => ` | ||
| <div class="forecast-item"> | ||
| <span class="forecast-item-temp">${item.displayTemp}</span> | ||
| <img class="forecast-item-icon" src="${item.iconSrc}" alt="${item.iconAlt}" /> | ||
| <span class="forecast-item-time">${item.timeLabel}</span> | ||
| </div> | ||
| `, | ||
| ) | ||
| .join('') |
There was a problem hiding this comment.
renderForecastItems() builds HTML via innerHTML and interpolates item.iconAlt (from the weather API) directly into an attribute. Even if OpenWeatherMap normally returns safe strings, this is still untrusted input and can lead to HTML/attribute injection. Prefer creating elements and setting textContent/setAttribute, or escape/sanitize interpolated values before assigning to innerHTML.
| forecastItemsEl.innerHTML = items | |
| .map( | |
| (item) => ` | |
| <div class="forecast-item"> | |
| <span class="forecast-item-temp">${item.displayTemp}</span> | |
| <img class="forecast-item-icon" src="${item.iconSrc}" alt="${item.iconAlt}" /> | |
| <span class="forecast-item-time">${item.timeLabel}</span> | |
| </div> | |
| `, | |
| ) | |
| .join('') | |
| // Clear any existing forecast items | |
| forecastItemsEl.innerHTML = '' | |
| for (const item of items) { | |
| const itemEl = document.createElement('div') | |
| itemEl.className = 'forecast-item' | |
| const tempEl = document.createElement('span') | |
| tempEl.className = 'forecast-item-temp' | |
| tempEl.textContent = item.displayTemp | |
| const iconEl = document.createElement('img') | |
| iconEl.className = 'forecast-item-icon' | |
| iconEl.setAttribute('src', item.iconSrc) | |
| iconEl.setAttribute('alt', item.iconAlt) | |
| const timeEl = document.createElement('span') | |
| timeEl.className = 'forecast-item-time' | |
| timeEl.textContent = item.timeLabel | |
| itemEl.appendChild(tempEl) | |
| itemEl.appendChild(iconEl) | |
| itemEl.appendChild(timeEl) | |
| forecastItemsEl.appendChild(itemEl) | |
| } |
| optional: true | ||
| help_text: | | ||
| Override the default locale with a supported language code (e.g., en, fr, de). Defaults to English if not specified. | ||
| You can find the available locales here: https://momentjs.com/ |
There was a problem hiding this comment.
This manifest includes override_locale/override_coordinates, but the app also depends on settings not declared here: getTimeZone() supports override_timezone, and getMeasurementUnit() reads unit. Without declaring them, users can’t configure timezone override or imperial/metric units via settings. Consider adding override_timezone and unit (as in edge-apps/clock/screenly.yml) and either wire up override_coordinates or remove it.
| You can find the available locales here: https://momentjs.com/ | |
| You can find the available locales here: https://momentjs.com/ | |
| override_timezone: | |
| type: string | |
| default_value: '' | |
| title: Override Timezone | |
| optional: true | |
| help_text: | | |
| Enter an IANA timezone identifier (e.g., Europe/Berlin, America/New_York) to override the device timezone. | |
| If not specified, the app will use the device's configured timezone. | |
| unit: | |
| type: string | |
| default_value: metric | |
| title: Measurement Unit | |
| optional: true | |
| choices: | |
| - metric | |
| - imperial | |
| help_text: | | |
| Select which measurement system to use. "metric" shows temperatures in °C and wind speed in m/s or km/h. | |
| "imperial" shows temperatures in °F and wind speed in mph. |
| override_coordinates: | ||
| type: string | ||
| default_value: '' | ||
| title: Override Coordinates | ||
| optional: true | ||
| help_text: | | ||
| Enter a comma-separated pair of coordinates (e.g., 37.8267,-122.4233). If not specified, the app will attempt to use the device's coordinates. | ||
| override_locale: | ||
| type: string | ||
| default_value: 'en' | ||
| title: Override Locale | ||
| optional: true | ||
| help_text: | | ||
| Override the default locale with a supported language code (e.g., en, fr, de). Defaults to English if not specified. | ||
| You can find the available locales here: https://momentjs.com/ |
There was a problem hiding this comment.
Same as screenly.yml: settings declared here don’t include override_timezone/unit even though the app reads them (via getTimeZone() and getMeasurementUnit()), and override_coordinates isn’t used by the app. Align the manifest settings with what the app actually supports.
| export async function getCurrentWeather( | ||
| lat: number, | ||
| lng: number, | ||
| tz: string, | ||
| ): Promise<CurrentWeatherData | null> { | ||
| try { | ||
| const apiKey = getSetting<string>('openweathermap_api_key') | ||
| if (!apiKey) { | ||
| return null | ||
| } | ||
|
|
||
| const unit = getMeasurementUnit() | ||
|
|
||
| const response = await fetch( | ||
| `https://api.openweathermap.org/data/2.5/weather?lat=${lat}&lon=${lng}&units=${unit}&appid=${apiKey}`, | ||
| ) | ||
|
|
||
| if (!response.ok) { | ||
| console.warn( | ||
| 'Failed to get weather data: OpenWeatherMap API responded with', | ||
| response.status, | ||
| response.statusText, | ||
| ) | ||
| return null | ||
| } | ||
|
|
||
| const data = await response.json() | ||
|
|
||
| if (!isValidWeatherResponse(data)) { | ||
| return null | ||
| } | ||
|
|
||
| const temperature = Math.round(data.main.temp) | ||
| const tempHigh = Math.round(data.main.temp_max) | ||
| const tempLow = Math.round(data.main.temp_min) | ||
| const weatherId = data.weather?.[0]?.id ?? null | ||
|
|
||
| if (!weatherId) { | ||
| return null | ||
| } | ||
|
|
||
| const dt = Math.floor(Date.now() / 1000) | ||
| const description = data.weather?.[0]?.description || '' | ||
| const iconSrc = getIconSrc(weatherId, dt, tz) | ||
| const iconAlt = description || 'Weather icon' | ||
|
|
||
| const tempSymbol = unit === 'imperial' ? '°F' : '°C' | ||
|
|
||
| return { | ||
| temperature, | ||
| weatherId, | ||
| description: capitalizeFirstLetter(description), | ||
| iconSrc, | ||
| iconAlt, | ||
| tempHigh, | ||
| tempLow, | ||
| displayTemp: `${temperature}${tempSymbol}`, | ||
| } | ||
| } catch (error) { | ||
| console.warn('Failed to get weather data:', error) | ||
| return null | ||
| } | ||
| } | ||
|
|
||
| // Get hourly forecast (next 8 entries from 3-hour forecast) | ||
| export async function getHourlyForecast( | ||
| lat: number, | ||
| lng: number, | ||
| tz: string, | ||
| locale: string, | ||
| ): Promise<ForecastItem[]> { | ||
| try { | ||
| const apiKey = getSetting<string>('openweathermap_api_key') | ||
| if (!apiKey) { | ||
| return [] | ||
| } | ||
|
|
||
| const unit = getMeasurementUnit() | ||
|
|
||
| const response = await fetch( | ||
| `https://api.openweathermap.org/data/2.5/forecast?lat=${lat}&lon=${lng}&units=${unit}&cnt=8&appid=${apiKey}`, | ||
| ) | ||
|
|
||
| if (!response.ok) { | ||
| console.warn( | ||
| 'Failed to get forecast data: OpenWeatherMap API responded with', | ||
| response.status, | ||
| response.statusText, | ||
| ) | ||
| return [] | ||
| } | ||
|
|
||
| const data = await response.json() | ||
|
|
||
| if (data.cod !== '200' || !Array.isArray(data.list)) { | ||
| return [] | ||
| } | ||
|
|
||
| return data.list.map( | ||
| ( | ||
| item: { | ||
| dt: number | ||
| main: { temp: number } | ||
| weather: { id: number; description: string }[] | ||
| }, | ||
| index: number, | ||
| ) => { | ||
| const temperature = Math.round(item.main.temp) | ||
| const weatherId = item.weather?.[0]?.id ?? 800 | ||
| const description = item.weather?.[0]?.description || 'Weather' | ||
| const iconSrc = getIconSrc(weatherId, item.dt, tz) | ||
|
|
||
| const timeLabel = | ||
| index === 0 | ||
| ? 'NOW' | ||
| : new Intl.DateTimeFormat(locale, { | ||
| hour: '2-digit', | ||
| minute: '2-digit', | ||
| hour12: false, | ||
| timeZone: tz, | ||
| }).format(new Date(item.dt * 1000)) | ||
|
|
||
| return { | ||
| temperature, | ||
| iconSrc, | ||
| iconAlt: description, | ||
| timeLabel, | ||
| displayTemp: `${temperature}°`, | ||
| } | ||
| }, | ||
| ) | ||
| } catch (error) { | ||
| console.warn('Failed to get forecast data:', error) | ||
| return [] | ||
| } | ||
| } |
There was a problem hiding this comment.
No unit tests are added for the new weather parsing/mapping logic (getCurrentWeather, getHourlyForecast, icon key mapping). The repo already has Bun unit tests for similar weather logic (e.g., edge-apps/clock/src/weather.test.ts), so adding tests here would help prevent regressions (especially around cod handling, 0° temps, and icon key mapping).
| const forecast = await getHourlyForecast(latitude, longitude, tz, locale) | ||
|
|
||
| if (forecast.length > 0) { | ||
| renderForecastItems(forecast) | ||
| } else { | ||
| hideForecastCard() | ||
| } |
There was a problem hiding this comment.
If the forecast request fails once, hideForecastCard() sets display: none permanently, and the card is never shown again even if a later refresh successfully returns forecast items (because the success path only renders items). Consider restoring the card display when forecast.length > 0 (e.g., clear the inline style or set display back to its default).
- Add getCoordinates() wrapper function to check override_coordinates setting - Parses comma-separated coordinates and validates before using - Falls back to metadata.coordinates if override is not specified Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
1 participant
User description
Summary
Redesigned weather app using TypeScript, edge-apps-library utilities, and web components. Includes current weather display and 8-item hourly forecast with glassmorphic design matching Figma specifications.
PR Type
Enhancement, Documentation
Description
Add redesigned weather edge app UI
Fetch current + 8-item forecast
Reverse-geocode coordinates to city name
Add manifests, scripts, and README
Diagram Walkthrough
File Walkthrough
6 files
Reverse-geocode coordinates into displayed city nameInitialize app, render weather, refresh periodicallyMap icon keys to bundled SVG assetsFetch and format current weather and forecastImplement glassmorphic layout and responsive stylesAdd app markup with header and forecast slots3 files
Add unit setting helper with defaultAdd manifest with API key and analytics settingsAdd QC manifest mirroring production settings1 files
Document settings and local build commands1 files
Configure bun scripts, tooling, and workspace deps