feat: add HVAC specialty-tank components (heat pump, hydraulic separator, heat exchanger) - #1054
feat: add HVAC specialty-tank components (heat pump, hydraulic separator, heat exchanger)#1054ludvigovrevik wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a reusable specialty-tank Lit base with medium fills, badges, tags, alert frames, and split geometries. Adds heat-exchanger, heat-pump, and hydraulic-separator elements with custom icons and Storybook stories for default, medium, badge, and alert states. ChangesSpecialty tank automation tiles
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Storybook
participant SpecialtyTile
participant ObcAbstractSpecialtyTank
participant ObcAlertFrame
Storybook->>SpecialtyTile: Bind story args
SpecialtyTile->>ObcAbstractSpecialtyTank: Render icon and split mode
ObcAbstractSpecialtyTank->>ObcAlertFrame: Render overlay when alert is enabled
ObcAbstractSpecialtyTank-->>Storybook: Return composed tile
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
The demo can be viewed at https://openbridge-next-demo--1054-feat-add-hvac-specialty-tan-28wedioc.web.app |
|
The storybook can be viewed at https://openbridge-next-storybook--1054-feat-add-hvac-specialt-v6zgh216.web.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/openbridge-webcomponents/src/automation/specialty-tank/abstract-specialty-tank.ts (4)
158-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate slot-change detection logic.
_onBadgesSlotChangeand_onTagSlotChangeimplement identical "has meaningful assigned content" logic, differing only in which@statefield they write. Extract a shared helper.♻️ Proposed refactor
- private _onBadgesSlotChange(e: Event): void { - const slot = e.target as HTMLSlotElement; - this._hasBadges = slot - .assignedNodes({flatten: true}) - .some( - (n) => - n.nodeType === Node.ELEMENT_NODE || - (n.nodeType === Node.TEXT_NODE && !!n.textContent?.trim()) - ); - } - - private _onTagSlotChange(e: Event): void { - const slot = e.target as HTMLSlotElement; - this._hasTagSlot = slot - .assignedNodes({flatten: true}) - .some( - (n) => - n.nodeType === Node.ELEMENT_NODE || - (n.nodeType === Node.TEXT_NODE && !!n.textContent?.trim()) - ); - } + private _slotHasContent(e: Event): boolean { + const slot = e.target as HTMLSlotElement; + return slot + .assignedNodes({flatten: true}) + .some( + (n) => + n.nodeType === Node.ELEMENT_NODE || + (n.nodeType === Node.TEXT_NODE && !!n.textContent?.trim()) + ); + } + + private _onBadgesSlotChange(e: Event): void { + this._hasBadges = this._slotHasContent(e); + } + + private _onTagSlotChange(e: Event): void { + this._hasTagSlot = this._slotHasContent(e); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openbridge-webcomponents/src/automation/specialty-tank/abstract-specialty-tank.ts` around lines 158 - 178, Extract the duplicated assigned-content check from _onBadgesSlotChange and _onTagSlotChange into a shared helper that accepts an HTMLSlotElement and returns whether it has meaningful assigned nodes. Update both handlers to use the helper while preserving their respective _hasBadges and _hasTagSlot assignments.
297-311: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGeneric
'Tank'aria-label fallback doesn't distinguish equipment type.When
tagis unset, all three subclasses (heat pump, hydraulic separator, heat exchanger) fall back to the same genericaria-label="Tank", giving screen-reader users no indication of which equipment tile they're interacting with.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openbridge-webcomponents/src/automation/specialty-tank/abstract-specialty-tank.ts` around lines 297 - 311, Update the aria-label in the render template of the abstract specialty tank component to use an equipment-specific fallback when tag is unset, so heat pump, hydraulic separator, and heat exchanger subclasses expose distinct accessible names while preserving the configured tag when present.
87-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer TypeScript
abstractover runtime throw for unimplemented getters.Since this class isn't a
@customElement(never directly instantiated), it can be declaredabstract class ObcAbstractSpecialtyTank extends LitElementwithabstract get equipmentIcon(): TemplateResult;andabstract get splitMode(): SpecialtyTankSplitMode;. This catches missing overrides at compile time instead of only at render-time.♻️ Proposed refactor
-export class ObcAbstractSpecialtyTank extends LitElement { +export abstract class ObcAbstractSpecialtyTank extends LitElement { ... - get equipmentIcon(): TemplateResult { - throw new Error('Method "equipmentIcon" must be implemented in subclass'); - } - - get splitMode(): SpecialtyTankSplitMode { - throw new Error('Method "splitMode" must be implemented in subclass'); - } + abstract get equipmentIcon(): TemplateResult; + + abstract get splitMode(): SpecialtyTankSplitMode;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openbridge-webcomponents/src/automation/specialty-tank/abstract-specialty-tank.ts` around lines 87 - 93, Declare ObcAbstractSpecialtyTank as an abstract class and mark its equipmentIcon and splitMode getters abstract, removing their runtime error-throwing implementations so subclasses must provide both members at compile time.
36-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBase-class JSDoc lacks the mandated structure.
The class doc is a short overview but omits the required Slots section (the class renders
badges,tag,alert-icon,alert-label,alert-timerslots), usage guidance, and features/variants. Since this is an abstract base whose description is meant to be reused/overridden in the concrete subclasses' Storybook docs, it should carry the full structured documentation.As per coding guidelines: "For Web Components, document all content slots in a Slots section, including each slot's name, when it renders, and its purpose; omit the section only if the component does not use slots" and "place a JSDoc block on the class or module and structure it with an overview, features/variants, usage guidance, slots/content, events, best practices."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openbridge-webcomponents/src/automation/specialty-tank/abstract-specialty-tank.ts` around lines 36 - 48, Expand the class-level JSDoc for the abstract specialty-tank base with the required structured sections: overview, features/variants, usage guidance, and slots/content. Document the badges, tag, alert-icon, alert-label, and alert-timer slots, including when each renders and its purpose; include events and best practices when applicable, and ensure the documentation is suitable for reuse by concrete subclasses.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/openbridge-webcomponents/src/automation/heat-exchanger/heat-exchanger.ts`:
- Around line 36-45: Reorder the JSDoc tag blocks so the Ignition tags and `@beta`
appear before the final slot/event-only block in heat-exchanger.ts (lines
36-45), heat-pump.ts (lines 36-45), and hydraulic-separator.ts (lines 36-45).
Preserve all existing tags and ensure each component’s JSDoc ends with only its
`@slot` and any `@fires/`@event tags.
---
Nitpick comments:
In
`@packages/openbridge-webcomponents/src/automation/specialty-tank/abstract-specialty-tank.ts`:
- Around line 158-178: Extract the duplicated assigned-content check from
_onBadgesSlotChange and _onTagSlotChange into a shared helper that accepts an
HTMLSlotElement and returns whether it has meaningful assigned nodes. Update
both handlers to use the helper while preserving their respective _hasBadges and
_hasTagSlot assignments.
- Around line 297-311: Update the aria-label in the render template of the
abstract specialty tank component to use an equipment-specific fallback when tag
is unset, so heat pump, hydraulic separator, and heat exchanger subclasses
expose distinct accessible names while preserving the configured tag when
present.
- Around line 87-93: Declare ObcAbstractSpecialtyTank as an abstract class and
mark its equipmentIcon and splitMode getters abstract, removing their runtime
error-throwing implementations so subclasses must provide both members at
compile time.
- Around line 36-48: Expand the class-level JSDoc for the abstract
specialty-tank base with the required structured sections: overview,
features/variants, usage guidance, and slots/content. Document the badges, tag,
alert-icon, alert-label, and alert-timer slots, including when each renders and
its purpose; include events and best practices when applicable, and ensure the
documentation is suitable for reuse by concrete subclasses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7965d583-922b-49a2-9b6c-0e60f483d362
📒 Files selected for processing (8)
packages/openbridge-webcomponents/src/automation/heat-exchanger/heat-exchanger.stories.tspackages/openbridge-webcomponents/src/automation/heat-exchanger/heat-exchanger.tspackages/openbridge-webcomponents/src/automation/heat-pump/heat-pump.stories.tspackages/openbridge-webcomponents/src/automation/heat-pump/heat-pump.tspackages/openbridge-webcomponents/src/automation/hydraulic-separator/hydraulic-separator.stories.tspackages/openbridge-webcomponents/src/automation/hydraulic-separator/hydraulic-separator.tspackages/openbridge-webcomponents/src/automation/specialty-tank/abstract-specialty-tank.tspackages/openbridge-webcomponents/src/automation/specialty-tank/specialty-tank.css
| * @slot badges - Custom badges, overriding the enum-driven defaults. | ||
| * @slot tag - Text or element replacing the `tag` property readout. | ||
| * @slot alert-icon - Custom icon for the alert frame. | ||
| * @slot alert-label - Label for the alert frame. | ||
| * @slot alert-timer - Timer for the alert frame. | ||
| * | ||
| * @ignition-base-width: 90px | ||
| * @ignition-base-height: 163px | ||
| * @ignition-center-horizontal | ||
| * @beta |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
End each component JSDoc with the slot/event block. The @ignition-* and @beta tags currently follow @slot tags, so the required final tag block is not slot/event-only.
packages/openbridge-webcomponents/src/automation/heat-exchanger/heat-exchanger.ts#L36-L45: move the Ignition and beta tags before a final@slotblock.packages/openbridge-webcomponents/src/automation/heat-pump/heat-pump.ts#L36-L45: move the Ignition and beta tags before a final@slotblock.packages/openbridge-webcomponents/src/automation/hydraulic-separator/hydraulic-separator.ts#L36-L45: move the Ignition and beta tags before a final@slotblock.
As per coding guidelines, “append a final tag block containing only @slot and @fires/@event tags.”
📍 Affects 3 files
packages/openbridge-webcomponents/src/automation/heat-exchanger/heat-exchanger.ts#L36-L45(this comment)packages/openbridge-webcomponents/src/automation/heat-pump/heat-pump.ts#L36-L45packages/openbridge-webcomponents/src/automation/hydraulic-separator/hydraulic-separator.ts#L36-L45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/openbridge-webcomponents/src/automation/heat-exchanger/heat-exchanger.ts`
around lines 36 - 45, Reorder the JSDoc tag blocks so the Ignition tags and
`@beta` appear before the final slot/event-only block in heat-exchanger.ts (lines
36-45), heat-pump.ts (lines 36-45), and hydraulic-separator.ts (lines 36-45).
Preserve all existing tags and ensure each component’s JSDoc ends with only its
`@slot` and any `@fires/`@event tags.
Source: Coding guidelines
Heat pump, hydraulic separator, and heat exchanger share one abstract base (ObcAbstractSpecialtyTank); subclasses override only the center icon and the hot/cold split geometry. Layout, tokens, divider, and glyph colors verified 1:1 against Figma nodes 18907-47623 / 18907-47658 / 18907-47693 in both hasMedium states. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Baselines generated through the CI-matching Docker image and verified stable with a second run. WithBadges stories switch the alert badge to alert-silenced, matching the default badge set on the Figma tiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Windows-generated __vis__/win32 snapshots are local-only (CI compares the linux baselines), mirroring the existing darwin ignore. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- declare the base abstract with abstract getters (compile-time check) - equipment-specific aria-label fallback via abstract equipmentName - deduplicate slot-content detection into a shared helper - full structured JSDoc on the base; end subclass JSDoc with slot tags Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The generated React wrapper passes every manifest class to createComponent as Constructor<T>; an abstract constructor is not assignable and broke the wrapper build. Revert to the runtime-throw override points used by ObcAbstractAutomationButton and document why. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Documents the Figma role mapping (Mode / Command / Alert badge order), the tag readout, and the alert overlay inline so the manifest and Storybook autodocs carry per-property descriptions, per the inline property-doc policy from #1046. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a977093 to
f043274
Compare
1 participant
Summary
Adds three new automation instrument tiles that share one abstract base class (the pump/fan/motor pattern):
obc-heat-pumpobi-heatpumpobc-hydraulic-separatorobi-hydraulic-separatorobc-heat-exchangerobi-heatexhangerObcAbstractSpecialtyTank(src/automation/specialty-tank/, not a custom element) owns the frame, badge row, hot/cold medium fill, corner glyphs, icon frame, tag readout, and alert-frame overlay. Each subclass overrides two getters:equipmentIconandsplitMode.Figma (OpenBridge 6.1)
Implemented 1:1 against the design nodes, both
hasMediumstates, via the Figma MCP:Details taken from Figma: 4px state-container padding + 2px value-padding gap, 6px halo radius, always-visible 4px divider with 1px
--instrument-frame-tertiary-coloredges, 4px-inset content area with 4px radius, glyphs--element-inactive-colorwhen grey /--base-red-500+--base-blue-500when medium, equipment icon--element-active-color. First consumer of the--automation-components-tanks-specialty-tanks-*tokens.API
medium(false),showMediumIcons(true),showTag(true) +tag— mirroring the Figma propshasMedium/hasIcons/hasIdTagObcAbstractAutomationButtonenums (badgeControl,badgeInterlock= Figma Duty,badgeCommandLocked,badgeAlert) with abadgesslot override; the row auto-hides when empty (FigmahasBadges=falsedefault)obc-automation-tank:alert,alertFrameType/Thickness/Status,showAlertCategoryIcon,showAlertIcon+alert-icon/alert-label/alert-timerslotsTesting
typecheck,eslint,lit-analyzer,lint:variablesall greenPlaywright Testsrun (win32 renders differ from CI)🤖 Generated with Claude Code
Summary by CodeRabbit