Feature/zone icons#695
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a shared Zone icon API to node-homey-lib, backed by a JSON manifest, including legacy icon ID resolution and a small test suite to validate the new public helpers.
Changes:
- Introduces
lib/ZonewithgetIcons()andresolveIcon()(manifest-driven, with legacy ID mapping). - Exposes
getZoneIcons()/resolveZoneIcon()(andZone) from the rootindex.js. - Adds
assets/zone/icons.jsonmanifest andtest/zone.jscoverage for active/legacy/unknown icon resolution.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/zone.js | Adds tests covering active icon enumeration, on-disk asset presence, and legacy/unknown resolution behavior. |
| lib/Zone/index.js | Implements manifest-backed Zone icon listing, caching, legacy ID remapping, and descriptor creation. |
| index.js | Exports Zone and binds new top-level helpers getZoneIcons / resolveZoneIcon. |
| assets/zone/icons.json | Defines the default icon, active icons list, and legacy icon remapping rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static _createIconDescriptor({ | ||
| id, | ||
| fileName, | ||
| }) { | ||
| const descriptor = { |
There was a problem hiding this comment.
The svg file names in both Sketch as in this file should use - not _
There was a problem hiding this comment.
Updated in commit 74a9333: all zone SVG filenames and manifest references now use hyphens instead of underscores.
There was a problem hiding this comment.
@HarwinBorger we cannot just change these file paths, that is a breaking change for consumers of this library. I don't think that's worth it.
There was a problem hiding this comment.
As discussed with @HarwinBorger privately on Slack, I agree that the naming is currently inconsistent. However, I also agree with @nozols that changing this would be a breaking change and is out of scope for this PR.
If we want to make the naming more consistent, we should first inventory which consumers rely on the current naming and check whether we can change this safely in separate PRs.
@HarwinBorger could you revert your changes for this PR?
There was a problem hiding this comment.
I added these icons like two weeks ago so I doubt anyone uses them at this point.
Just bump a major version :)
| * @param {string | null | undefined} icon | ||
| * @returns {ZoneIconDescriptor} | ||
| */ | ||
| static resolveIcon(icon) { |
There was a problem hiding this comment.
@copilot is this the best name for the function?
There was a problem hiding this comment.
Kept the name resolveIcon (current HEAD 74a9333) because it stays concise within Zone and aligns with the public resolveZoneIcon export; it resolves an icon id to the descriptor.
Agent-Logs-Url: https://github.com/athombv/node-homey-lib/sessions/80694955-b0c5-4d6e-ad42-bafe6aa2d036 Co-authored-by: HarwinBorger <3984675+HarwinBorger@users.noreply.github.com>
| it('returns active zone icons', function() { | ||
| const icons = HomeyLib.getZoneIcons(); | ||
|
|
||
| assert.strictEqual(icons.length, 47); |
There was a problem hiding this comment.
I'd prefer to check icons.length > 0 instead, then we don't have to update the tests if we add more icons.
Adds a shared Zone icon API backed by a JSON manifest, including legacy icon resolution and tests for the new public helpers.