feat(automation): add hydraulic valve shuffle components - #1059
Open
ludvigovrevik wants to merge 8 commits into
Open
feat(automation): add hydraulic valve shuffle components#1059ludvigovrevik wants to merge 8 commits into
ludvigovrevik wants to merge 8 commits into
Conversation
Add three components built on a shared shuffle-button base where the selected position thumb always occupies the fixed center slot and option thumbs redistribute around it: - obc-hydraulic-valve-4-3: 4/3 directional valve, 5 type variants - obc-hydraulic-valve-x-2: 2/2, 3/2 and 4/2 two-position valves - obc-hydraulic-check-valve: static check valve symbol Controlled interaction (position-selected event), WAI-ARIA radio-group keyboard navigation, sizes derived from global size tokens so components scale with obc-component-size-* classes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ludvigovrevik
requested review from
jon-daeh,
tibnor and
ulrik-jo
as code owners
July 23, 2026 22:28
Contributor
|
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:
📝 WalkthroughWalkthroughAdds a reusable accessible shuffle-button base with layout helpers, styling, keyboard and event handling. Adds configurable 2-position and 4/3 hydraulic valves, a static check valve, custom-element typings, Storybook stories, and layout tests. ChangesHydraulic controls
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant HydraulicValve
participant ObcShuffleButtonBase
participant Storybook
User->>HydraulicValve: click thumb or press Arrow key
HydraulicValve->>ObcShuffleButtonBase: handle selection request
ObcShuffleButtonBase->>Storybook: dispatch position-selected
Storybook->>HydraulicValve: update selectedPosition
HydraulicValve->>ObcShuffleButtonBase: render selected position
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
The storybook can be viewed at https://openbridge-next-storybook--1059-feat-automation-add-hy-svngviui.web.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/openbridge-webcomponents/src/automation/hydraulic-check-valve/hydraulic-check-valve.ts (1)
8-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the required component documentation sections.
The class JSDoc has a summary and usage guidance, but it omits the required features/variants and best-practice/constraint guidance, including when this display-only component should not be used. Add those sections; there are no slots or events to document here.
As per coding guidelines, component JSDoc must include overview, features/variants, usage guidance, slots/content, events, and 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/hydraulic-check-valve/hydraulic-check-valve.ts` around lines 8 - 21, Expand the class JSDoc for the hydraulic check-valve component with the required features/variants and best-practices/constraints sections, explicitly stating when the display-only component should not be used. Retain the existing overview and usage guidance, and document that there are no slots/content or events.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/hydraulic-valve-x-2/hydraulic-valve-x-2.ts`:
- Around line 23-49: Update the component JSDoc in
packages/openbridge-webcomponents/src/automation/hydraulic-valve-x-2/hydraulic-valve-x-2.ts
lines 23-49 and
packages/openbridge-webcomponents/src/automation/hydraulic-valve-4-3/hydraulic-valve-4-3.ts
lines 26-52: add practical controlled-selection best-practice guidance, move
`@alpha` before the final tag block, and ensure that final block contains only the
existing `@fires/`@event and `@slot` tags.
In
`@packages/openbridge-webcomponents/src/automation/shuffle-button/shuffle-button-base.ts`:
- Around line 46-49: Update the ariaLabel property in the shuffle-button base
component to use a meaningful non-empty default accessible name, ensuring the
rendered radiogroup is labeled when consumers do not provide a value. Preserve
consumer overrides and apply the default at the base property rather than
requiring changes to each concrete valve component.
- Around line 34-35: Remove the `@fires` position-selected declaration from the
abstract shuffle-button base class and add it to each concrete `@customElement`
class that extends it. Keep the existing event name and description unchanged,
placing the manifest tag in each concrete component’s class documentation.
In
`@packages/openbridge-webcomponents/src/automation/shuffle-button/shuffle-layout.ts`:
- Around line 7-9: Update clampPosition to normalize a NaN position to the
fallback position before rounding and clamping, ensuring it returns a valid
index within the available range; add a regression test covering NaN input and
the resulting valid position.
---
Nitpick comments:
In
`@packages/openbridge-webcomponents/src/automation/hydraulic-check-valve/hydraulic-check-valve.ts`:
- Around line 8-21: Expand the class JSDoc for the hydraulic check-valve
component with the required features/variants and best-practices/constraints
sections, explicitly stating when the display-only component should not be used.
Retain the existing overview and usage guidance, and document that there are no
slots/content or events.
🪄 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: 3adfd624-8954-47e2-9fa7-e2aeec6d0dc4
📒 Files selected for processing (11)
packages/openbridge-webcomponents/src/automation/hydraulic-check-valve/hydraulic-check-valve.csspackages/openbridge-webcomponents/src/automation/hydraulic-check-valve/hydraulic-check-valve.stories.tspackages/openbridge-webcomponents/src/automation/hydraulic-check-valve/hydraulic-check-valve.tspackages/openbridge-webcomponents/src/automation/hydraulic-valve-4-3/hydraulic-valve-4-3.stories.tspackages/openbridge-webcomponents/src/automation/hydraulic-valve-4-3/hydraulic-valve-4-3.tspackages/openbridge-webcomponents/src/automation/hydraulic-valve-x-2/hydraulic-valve-x-2.stories.tspackages/openbridge-webcomponents/src/automation/hydraulic-valve-x-2/hydraulic-valve-x-2.tspackages/openbridge-webcomponents/src/automation/shuffle-button/shuffle-button-base.tspackages/openbridge-webcomponents/src/automation/shuffle-button/shuffle-button.csspackages/openbridge-webcomponents/src/automation/shuffle-button/shuffle-layout.spec.tspackages/openbridge-webcomponents/src/automation/shuffle-button/shuffle-layout.ts
Comment on lines
+34
to
+35
| * @fires position-selected {PositionSelectedEvent} Position requested by the user. | ||
| * @ignore |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move the event manifest tag to concrete components.
@fires on this abstract base violates the component-documentation contract; document position-selected on each concrete @customElement class instead.
As per coding guidelines, “@slot and @fires tags must live on the concrete @customElement class.”
🤖 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/shuffle-button/shuffle-button-base.ts`
around lines 34 - 35, Remove the `@fires` position-selected declaration from the
abstract shuffle-button base class and add it to each concrete `@customElement`
class that extends it. Keep the existing event name and description unchanged,
placing the manifest tag in each concrete component’s class documentation.
Source: Coding guidelines
Comment on lines
+7
to
+9
| export function clampPosition(positionCount: number, position: number): number { | ||
| return Math.min(Math.max(Math.round(position), 0), positionCount - 1); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize NaN before clamping.
A public selectedPosition = NaN produces NaN here, leaving every radio unselected/non-tabbable and emitting an invalid CSS offset. Treat NaN as a fallback position and add a regression test.
Proposed fix
export function clampPosition(positionCount: number, position: number): number {
- return Math.min(Math.max(Math.round(position), 0), positionCount - 1);
+ const roundedPosition = Number.isNaN(position) ? 0 : Math.round(position);
+ return Math.min(Math.max(roundedPosition, 0), positionCount - 1);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function clampPosition(positionCount: number, position: number): number { | |
| return Math.min(Math.max(Math.round(position), 0), positionCount - 1); | |
| } | |
| export function clampPosition(positionCount: number, position: number): number { | |
| const roundedPosition = Number.isNaN(position) ? 0 : Math.round(position); | |
| return Math.min(Math.max(roundedPosition, 0), positionCount - 1); | |
| } |
🤖 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/shuffle-button/shuffle-layout.ts`
around lines 7 - 9, Update clampPosition to normalize a NaN position to the
fallback position before rounding and clamping, ensuring it returns a valid
index within the available range; add a regression test covering NaN input and
the resulting valid position.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Normalize NaN selectedPosition to position 0 (+ regression test) - Non-empty default ariaLabel on the base class - Move @fires manifest tag off the abstract base; keep it on concrete components only, and place @Alpha before the final tag block Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Speed the selection transition up to 100ms ease-out. Suppress hover feedback (via --obc-can-hover) from click until the next pointer move, because browsers keep :hover on the thumb that shuffles away from under a stationary pointer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The lit-labs wrapper generators resolve event types declared in the component's own module; a type re-exported from the shared base is emitted into the Vue wrapper without an import, breaking the vue and demo builds. Declare PositionSelectedDetail locally in each valve component (matching the progress-button pattern) and reference it from the @fires tag. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The demo can be viewed at https://openbridge-next-demo--1059-feat-automation-add-hydraul-63mnrlzb.web.app |
The generated React wrapper passes every manifest class to createComponent as Constructor<T>; an abstract constructor is not assignable. Same approach as the specialty-tank base: concrete class with default implementations that subclasses override. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add a vertical boolean to the shuffle-button base and the hydraulic valve components, per the Figma shuffle button Direction=Vertical variants: positions stack top to bottom, the track and thumb geometry transpose, and position symbols rotate 90deg counter-clockwise to match a vertical flow path. Linux visual baselines generated via the project Docker image and verified stable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds the three hydraulic valve symbols from OpenBridge 6.1: the 4/3 directional valve, the X/2 valve (2/2, 3/2, 4/2) and the check valve.
Try them in the Storybook preview: Valve 4/3 · Valve X/2 · Check valve
@tibnor — could you review the code here? Three design choices I'd especially like your take on:
ObcShuffleButtonBase, and the valves are thin subclasses that just supply their icons. Is that the right split, or would you prefer three independent components?position-selected— the symbol doesn't move until the app setsselectedPosition. Reasoning: the actual valve position is plant feedback, and the UI shouldn't show a position the device never reached. (The stories wire the event back to the property so it feels live in Storybook.) OK, or should it update itself?prefers-reduced-motion). The Figma frames are all marked WIP and don't specify motion, so this is a guess — happy to remove it.What's inside
obc-hydraulic-valve-4-3— 3 positions, 5 type variants for the center symbolobc-hydraulic-valve-x-2— 2 positions, types2/2,3/2,4/2obc-hydraulic-check-valve— static single symbol, display-onlyobc-component-size-*classes; colors are the automation pipe/device tokens; every icon already existed in the library (obi-hydraulic-*) — no new SVGwipwhile the design is WIP in FigmaFigma: 4/3 valve · X/2 valve · check valve
Testing
🤖 Generated with Claude Code
Summary by CodeRabbit
position-selectedevent support.