feat(ui-next): Adds toggle state for ToolButton and Crosshair example#5914
feat(ui-next): Adds toggle state for ToolButton and Crosshair example#5914dan-rukas wants to merge 5 commits intoOHIF:masterfrom
Conversation
✅ Deploy Preview for ohif-dev canceled.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const isOn = currentMode === Enums.ToolModes.Passive || | ||
| currentMode === Enums.ToolModes.Enabled; |
There was a problem hiding this comment.
Active mode excluded from isOn check
The isOn detection uses Passive || Enabled but omits Active. If Crosshairs is activated through an alternative path (e.g., a keyboard binding or another command that calls setToolActive), the button will appear un-toggled even though the tool is visually active in the viewports. The corresponding command logic (line 1054–1055 in commandsModule.ts) shares the same omission, so clicking the button when Crosshairs is already Active would try to enable Passive mode again instead of disabling it, leaving the crosshairs on.
For consistency with toggleActiveDisabledToolbar, consider including Active:
| const isOn = currentMode === Enums.ToolModes.Passive || | |
| currentMode === Enums.ToolModes.Enabled; | |
| const isOn = currentMode === Enums.ToolModes.Passive || | |
| currentMode === Enums.ToolModes.Enabled || | |
| currentMode === Enums.ToolModes.Active; |
There was a problem hiding this comment.
The omission of Active is intentional.
See the PR message above - this implementation scopes the toolbar button to a pure on/off toggle (Passive ↔ Disabled). The crosshair tool is never set to Active through the toolbar. Active mode will be handled separately at the viewport level in @sedghi's upcoming work via a modifier key interaction. The toolbar button intentionally stays in its toggled appearance during that interaction. Any changes to support Active mode would be part of that follow-up work.
| const isOn = currentMode === Enums.ToolModes.Passive || | ||
| currentMode === Enums.ToolModes.Enabled; |
There was a problem hiding this comment.
Active mode excluded from isOn check (same as evaluator)
This mirrors the omission in the evaluator: if Crosshairs is somehow in Active mode (keyboard shortcut or another command), isOn will be false, and clicking the toggle button will call setToolPassive instead of setToolDisabled, leaving crosshairs active without feedback.
The same fix applies here:
| const isOn = currentMode === Enums.ToolModes.Passive || | |
| currentMode === Enums.ToolModes.Enabled; | |
| const isOn = currentMode === Enums.ToolModes.Passive || | |
| currentMode === Enums.ToolModes.Enabled || | |
| currentMode === Enums.ToolModes.Active; |
There was a problem hiding this comment.
Same reasoning as above. Active mode is out of scope for this PR and will be handled in follow-up work.
| name: 'evaluate.cornerstoneTool.crosshairToggle', | ||
| evaluate: ({ viewportId, button, disabledText }) => { | ||
| const toolGroup = toolGroupService.getToolGroupForViewport(viewportId); | ||
| if (!toolGroup) { | ||
| return; | ||
| } | ||
|
|
||
| const toolName = toolbarService.getToolNameForButton(button); | ||
| if (!toolGroup.hasTool(toolName)) { | ||
| return getDisabledState(disabledText); | ||
| } | ||
|
|
||
| const currentMode = toolGroup.getToolOptions(toolName).mode; | ||
| const isOn = currentMode === Enums.ToolModes.Passive || | ||
| currentMode === Enums.ToolModes.Enabled; | ||
|
|
||
| return { | ||
| disabled: false, | ||
| isActive: false, | ||
| isToggled: isOn, | ||
| icon: isOn ? 'tool-crosshair-checked' : 'tool-crosshair', | ||
| }; | ||
| }, |
There was a problem hiding this comment.
isOn detection logic
The isOn evaluation (Passive || Enabled) is copy-pasted identically into both evaluate.cornerstoneTool.crosshairToggle (here) and toggleCrosshairsToolbar in commandsModule.ts. When one changes (e.g., adding Active to the check), the other must be updated manually.
Consider extracting this into a shared helper, or at minimum leaving a comment cross-referencing the two sites so they stay in sync.
There was a problem hiding this comment.
The two checks should stay in sync. I added cross-reference comments in both locations explaining that they must agree on which modes count as "on," and noting that both need updating if Active mode support is added.
|
@sedghi Greptile's comments above focus on some of the Again, the Crosshairs implementation is for reference only, so feel free to modify, extend, etc. |
…nsistency in command naming and update related references in the toolbar buttons.
…users to set modifier keys for jump actions. Update User Preferences modal to display and manage mouse shortcuts, enhancing user experience with tool interactions.
Viewers
|
||||||||||||||||||||||||||||
| Project |
Viewers
|
| Branch Review |
feat/crosshair-and-toolbutton-toggle
|
| Run status |
|
| Run duration | 02m 23s |
| Commit |
|
| Committer | Alireza |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
37
|
| View all changes introduced in this branch ↗︎ | |
2 participants
Context
Adds new toggle state for ToolButton component and reference implementation for the Crosshair tool.
Review the deploy preview: https://crosshair-simple-toggle.netlify.app/
Changes & Results
ToolButton changes in ui-next:
isToggledvisual state to the ToolButton component for tools that persist when switching to another toolToolCrosshairCheckedicon (crosshair with checkmark badge)Crosshair Implementation (reference — can be extended)
The crosshair toggle demonstrates the new state with a minimal implementation:
disableOnPassiveset tofalseso crosshairs survive tool switchesThis implementation is intentionally scoped as a starting point. It can be extended to support additional interactions such as:
Changed files:
platform/ui-next/src/components/ToolButton/ToolButton.tsx— newisToggledpropplatform/ui-next/src/components/Icons/Sources/Tools.tsx— newToolCrosshairCheckediconplatform/ui-next/src/components/Icons/Icons.tsx— icon registrationextensions/cornerstone/src/commandsModule.ts—toggleCrosshairsToolbarcommandextensions/cornerstone/src/getToolbarModule.tsx—crosshairToggleevaluatormodes/basic/src/toolbarButtons.ts— crosshair button configmodes/basic/src/initToolGroups.ts—disableOnPassive: falseTesting
to checked variant with highlight color
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Greptile Summary
This PR introduces a new
isToggledvisual state to theToolButtoncomponent and implements the Crosshairs tool as its first reference use case, allowing users to toggle crosshairs on/off in MPR mode without changing the active tool. The implementation is well-structured and follows existing patterns in the codebase.Key changes:
ToolButtongains anisToggledprop with a distinct visual style (text-highlighton transparent background), correctly placed in the class-priority chain (disabled > isActive > isToggled > default)evaluate.cornerstoneTool.crosshairToggleevaluator returnsisToggledstate and swaps icons dynamically betweentool-crosshairandtool-crosshair-checkedtoggleCrosshairsToolbarcommand toggles the tool betweenPassiveandDisabledmodes across specified tool groupsdisableOnPassive: falsecorrectly prevents crosshairs from being swept away when another tool is activatedToolCrosshairCheckedSVG icon provides clear visual feedback for the toggled-on stateisOnguard in both the evaluator and the command omitsEnums.ToolModes.Active— if Crosshairs is ever set Active via a keyboard shortcut or another command path, the button will appear un-toggled and clicking will callsetToolPassiveinstead ofsetToolDisabled, leaving crosshairs visible without the user intending itisOnlogic is copy-pasted verbatim into both files, so the two must be kept in sync manuallyaria-pressedis absent from the inner<Button>inToolButton, meaning screen readers cannot announce the toggle state to assistive-technology usersConfidence Score: 3/5
Active-mode omission in both the command and evaluator can silently leave crosshairs visible when the user expects them to be toggled off.Activefrom theisOncheck in bothtoggleCrosshairsToolbarand the evaluator—is small but can produce incorrect toggle behavior in a realistic scenario (keyboard shortcut activating Crosshairs), and fixing it in one place without the other would still leave the bug. The duplication makes it easy to miss. These two fixes should be applied before merging.extensions/cornerstone/src/commandsModule.tsandextensions/cornerstone/src/getToolbarModule.tsx— both needActiveadded to theisOncheck and ideally share the logic.Important Files Changed
isToggledprop with a newtoggledClassesvisual state; the class precedence chain is correct, butaria-pressedis missing from the inner<Button>for accessibility.evaluate.cornerstoneTool.crosshairToggleevaluator correctly disables the button for non-MPR viewports, but excludesActivemode fromisOn, and duplicates toggle-state logic withcommandsModule.ts.toggleCrosshairsToolbaraction toggles the tool between Passive and Disabled across specified tool groups; sameActive-mode omission as the evaluator means the command misfires when crosshairs is Active.disableOnPassivetofalseso crosshairs survive tool switches — correct and intentional for the toggle-based UX.ToolCrosshairCheckedSVG icon added with a uniqueclipPathid; no collision with the existingToolCrosshairwhich uses no clip path.ToolCrosshairCheckedand thetool-crosshair-checkedstring key; no issues.Sequence Diagram
sequenceDiagram participant User participant ToolButton participant ToolbarService participant toggleCrosshairsToolbar participant ToolGroup participant crosshairToggleEvaluator User->>ToolButton: Click crosshair button ToolButton->>ToolbarService: onInteraction({ itemId: 'Crosshairs' }) ToolbarService->>toggleCrosshairsToolbar: execute({ itemId, toolGroupIds: ['mpr'] }) toggleCrosshairsToolbar->>ToolGroup: getToolGroup('mpr') toggleCrosshairsToolbar->>ToolGroup: getToolOptions('Crosshairs').mode alt isOn (Passive or Enabled) toggleCrosshairsToolbar->>ToolGroup: setToolDisabled('Crosshairs') else isOff (Disabled) toggleCrosshairsToolbar->>ToolGroup: setToolPassive('Crosshairs') end Note over ToolGroup: State change triggers re-evaluation crosshairToggleEvaluator->>ToolGroup: getToolGroupForViewport(viewportId) crosshairToggleEvaluator->>ToolGroup: getToolOptions('Crosshairs').mode crosshairToggleEvaluator-->>ToolButton: { isToggled, icon } ToolButton-->>User: Visual state update (icon swap + highlight color)Comments Outside Diff (1)
platform/ui-next/src/components/ToolButton/ToolButton.tsx, line 91-103 (link)aria-pressedattribute for accessibilityThe
data-toggledattribute is added to the wrapping<span>for DOM querying, but there is noaria-pressedon the<Button>itself. Withoutaria-pressed, screen readers cannot announce the toggle state change to assistive-technology users.Reviews (1): Last reviewed commit: "Added correct disabled text" | Re-trigger Greptile
(5/5) You can turn off certain types of comments like style here!