From 9b0d542526e9a135c116d88ce9565efae6a21764 Mon Sep 17 00:00:00 2001 From: Joe Boccanfuso <109477394+jbocce@users.noreply.github.com> Date: Fri, 6 Jan 2023 15:00:23 -0500 Subject: [PATCH] fix: rework inconsistencies in split button implementation(#3086) * feat: Combine the layout grid selector tool with the toggle MPR button into a split button. The primary button will always be the layout grid selector and the MPR toggle is in the drop down menu. This is the first step to later adding other (e.g. 3D) layout HPs to the menu. Summary of changes/fixes: - SplitButton now allows for its primary button to be any button component specified by a UI type. - SplitButton passes a className to its primary button component to dictate how it should be rendered based on its active/toggle status, hovering, menu visibility etc. - The default menu item renderer for the SplitButton, renders acitve toggle items correctly. - Items/buttons in the toolbar are now all the correct and same size. - Added ToolbarService.getButtonComponentForUIType to fetch a button component for a given UI type. - Spacing between toolbar items is now done at the Toolbar level to avoid double spacing and to ensure uniform spacing. - The drop down menu closes when clicking the primary button. - Show a mouse down click effect when clicking the primary button. Items to investigate still: The following are all slightly contrary to the design specs of the UI: - Is the click effect on the primary button satisfactory because with the hover border effect, the button appears slightly smaller during the mouse down/up operation? (MEDIUM) - On hover over the drop down button, the right-hand-side corners of the primary button appear rounded and NOT square. Also the hover over the drop down button border effects make the primary button appear smaller. (MEDIUM) - Should there be hover effects when hovering over either tooltip of the SplitButton? (LOW) * feat: Combine the layout grid selector tool with the toggle MPR button into a split button. Fixed broken e2e tests. data-cy and data-tool properties now passed down to primary button component. * feat: Combine the layout grid selector tool with the toggle MPR button into a split button. On drop down button hover, the primary button now stretches all the way right with no right border and with square corners in the top and bottom right as per the toolbar spec. The mouse down effect gives a full button look (i.e. no borders) with rounded corners all around and black text icon. * feat: Combine the layout grid selector tool with the toggle MPR button into a split button. Documented that the primary button of a split button can specify a UI type. * feat: Combine the layout grid selector tool with the toggle MPR button into a split button. Feedback from PR review to fix prettier formatting issue. * feat: Combine the layout grid selector tool with the toggle MPR button into a split button. Added TODOs as per Bill's request in the PR. * feat: Combine the layout grid selector tool with the toggle MPR button into a split button. Feedback from Alireza: reverted the layout selector and MPR tools to be separate toolbar items. Co-authored-by: Joe Boccanfuso --- .../src/utils/mpr/toggleMPRHangingProtocol.ts | 4 + extensions/default/src/Toolbar/Toolbar.tsx | 22 ++- .../src/Toolbar/ToolbarLayoutSelector.tsx | 44 +++-- extensions/default/src/getToolbarModule.tsx | 22 +-- modes/longitudinal/src/toolbarButtons.js | 3 +- .../services/ToolBarService/ToolBarService.ts | 6 + .../platform/extensions/modules/toolbar.md | 12 +- .../src/components/IconButton/IconButton.tsx | 4 +- .../components/SplitButton/SplitButton.tsx | 160 +++++++++--------- .../ToolbarButton/ToolbarButton.tsx | 5 +- 10 files changed, 155 insertions(+), 127 deletions(-) diff --git a/extensions/cornerstone/src/utils/mpr/toggleMPRHangingProtocol.ts b/extensions/cornerstone/src/utils/mpr/toggleMPRHangingProtocol.ts index cce8869a3..dfe18f5f4 100644 --- a/extensions/cornerstone/src/utils/mpr/toggleMPRHangingProtocol.ts +++ b/extensions/cornerstone/src/utils/mpr/toggleMPRHangingProtocol.ts @@ -45,6 +45,10 @@ export default function toggleMPRHangingProtocol({ ToolBarService, } = servicesManager.services; + // TODO Introduce a service to persist the state of the current hanging protocol/app. + // So all of the code to persist the state here will no longer be needed. Perhaps + // just the id of the current hanging protocol to toggle MPR off is needed. + const { activeViewportIndex, viewports, diff --git a/extensions/default/src/Toolbar/Toolbar.tsx b/extensions/default/src/Toolbar/Toolbar.tsx index 3a4bd8164..68a064754 100644 --- a/extensions/default/src/Toolbar/Toolbar.tsx +++ b/extensions/default/src/Toolbar/Toolbar.tsx @@ -1,4 +1,5 @@ import React, { useEffect, useState } from 'react'; +import classnames from 'classnames'; export default function Toolbar({ servicesManager }) { const { ToolBarService } = servicesManager.services; @@ -46,15 +47,18 @@ export default function Toolbar({ servicesManager }) { // These can... Trigger toolbar events based on updates? // Then sync using useEffect, or simply modify the state here? return ( - ToolBarService.recordInteraction(args)} - servicesManager={servicesManager} - /> + // The margin for separating the tools on the toolbar should go here and NOT in each individual component (button) item. + // This allows for the individual items to be included in other UI components where perhaps alternative margins are desired. +
+ ToolBarService.recordInteraction(args)} + servicesManager={servicesManager} + /> +
); })} diff --git a/extensions/default/src/Toolbar/ToolbarLayoutSelector.tsx b/extensions/default/src/Toolbar/ToolbarLayoutSelector.tsx index a2e4509cf..56efcda34 100644 --- a/extensions/default/src/Toolbar/ToolbarLayoutSelector.tsx +++ b/extensions/default/src/Toolbar/ToolbarLayoutSelector.tsx @@ -6,12 +6,18 @@ import { useViewportGrid, } from '@ohif/ui'; -function LayoutSelector({ rows, columns, servicesManager }) { +function LayoutSelector({ + rows, + columns, + className, + servicesManager, + ...rest +}) { const [isOpen, setIsOpen] = useState(false); const [disableSelector, setDisableSelector] = useState(false); const [viewportGridState, viewportGridService] = useViewportGrid(); - const { HangingProtocolService } = servicesManager.services; + const { HangingProtocolService, ToolBarService } = servicesManager.services; const closeOnOutsideClick = () => { if (isOpen) { @@ -24,12 +30,6 @@ function LayoutSelector({ rows, columns, servicesManager }) { HangingProtocolService.EVENTS.PROTOCOL_CHANGED, evt => { const { protocol } = evt; - - if (protocol.id === 'mpr') { - setDisableSelector(true); - } else { - setDisableSelector(false); - } } ); @@ -55,20 +55,42 @@ function LayoutSelector({ rows, columns, servicesManager }) { const onInteractionHandler = () => setIsOpen(!isOpen); const DropdownContent = isOpen ? OHIFLayoutSelector : null; + const onSelectionHandler = ({ numRows, numCols }) => { + // TODO Introduce a service to persist the state of the current hanging protocol/app. + + // TODO Here the layout change will amount to a change of hanging protocol as specified by the extension for this layout selector tool + // followed by the change of the grid itself. + if (HangingProtocolService.getActiveProtocol().protocol.id === 'mpr') { + ToolBarService.recordInteraction({ + groupId: 'MPR', + itemId: 'MPR', + interactionType: 'toggle', + commands: [ + { + commandName: 'toggleMPR', + commandOptions: {}, + context: 'CORNERSTONE', + }, + ], + }); + } + viewportGridService.setLayout({ numRows, numCols }); + }; + return ( { - viewportGridService.setLayout({ numRows, numCols }); - }} + onSelection={onSelectionHandler} /> ) } diff --git a/extensions/default/src/getToolbarModule.tsx b/extensions/default/src/getToolbarModule.tsx index 260ef111b..f29744d60 100644 --- a/extensions/default/src/getToolbarModule.tsx +++ b/extensions/default/src/getToolbarModule.tsx @@ -35,27 +35,7 @@ export default function getToolbarModule({ commandsManager, servicesManager }) { { name: 'ohif.toggle', defaultComponent: ToolbarButton, - requiredConfig: [], - optionalConfig: [], - requiredProps: [], - optionalProps: [], - clickHandler: (evt, clickedBtn, btnSectionName) => { - const { props } = clickedBtn; - const allButtons = toolbarService.getButtons(); - const thisButton = allButtons[clickedBtn.id]; - - // Set our clicked button to active - thisButton.props.isActive = !thisButton.props.isActive; - - // Run button logic/command - commandsManager.runCommand(props.commandName, props.commandOptions); - - // What if just toggled "content"? - // commandName OR content? - - // Set buttons & trigger notification - toolbarService.setButtons(allButtons); - }, + clickHandler: () => {}, }, ]; } diff --git a/modes/longitudinal/src/toolbarButtons.js b/modes/longitudinal/src/toolbarButtons.js index c6dcd101f..a8c8a8535 100644 --- a/modes/longitudinal/src/toolbarButtons.js +++ b/modes/longitudinal/src/toolbarButtons.js @@ -15,7 +15,7 @@ const { windowLevelPresets } = defaults; * @param {*} icon * @param {*} label */ -function _createButton(type, id, icon, label, commands, tooltip) { +function _createButton(type, id, icon, label, commands, tooltip, uiType) { return { id, icon, @@ -23,6 +23,7 @@ function _createButton(type, id, icon, label, commands, tooltip) { type, commands, tooltip, + uiType, }; } diff --git a/platform/core/src/services/ToolBarService/ToolBarService.ts b/platform/core/src/services/ToolBarService/ToolBarService.ts index 908899f21..3aa745bb2 100644 --- a/platform/core/src/services/ToolBarService/ToolBarService.ts +++ b/platform/core/src/services/ToolBarService/ToolBarService.ts @@ -287,4 +287,10 @@ export default class ToolBarService { componentProps: Object.assign({}, btn.props, props), }; } + + getButtonComponentForUIType(uiType: string) { + return uiType + ? this._buttonTypes()[uiType]?.defaultComponent ?? null + : null; + } } diff --git a/platform/docs/docs/platform/extensions/modules/toolbar.md b/platform/docs/docs/platform/extensions/modules/toolbar.md index 95ff04649..bf48fda6c 100644 --- a/platform/docs/docs/platform/extensions/modules/toolbar.md +++ b/platform/docs/docs/platform/extensions/modules/toolbar.md @@ -44,6 +44,11 @@ export default function getToolbarModule({ commandsManager, servicesManager }) { defaultComponent: ToolbarLayoutSelector, clickHandler: (evt, clickedBtn, btnSectionName) => {}, }, + { + name: 'ohif.toggle', + defaultComponent: ToolbarButton, + clickHandler: () => {}, + }, ]; } ``` @@ -129,8 +134,11 @@ There are three main types of toolbar buttons: You can use the `ohif.splitButton` type to build a button with extra tools in the dropdown. -- First you need to give your `primary` tool definition to the split button -- the `secondary` properties can be a simple arrow down (`chevron-down` icon) +- First you need to give your `primary` tool definition to the split button. The primary +tool can specify a `uiType` property which can be one of the button types returned by +`getToolbarModule` that is a variation of `ToolbarButton`. If `uiType` is omitted then +`ToolbarButton` is used by default. +- The `secondary` properties can be a simple arrow down (`chevron-down` icon) - For adding the extra tools add them to the `items` list. You can see below how `longitudinal` mode is using the available toolbarModule diff --git a/platform/ui/src/components/IconButton/IconButton.tsx b/platform/ui/src/components/IconButton/IconButton.tsx index 93bff34c5..d073443d7 100644 --- a/platform/ui/src/components/IconButton/IconButton.tsx +++ b/platform/ui/src/components/IconButton/IconButton.tsx @@ -140,8 +140,8 @@ const IconButton = ({ ref={buttonElement} onClick={handleOnClick} type={type} - data-cy={id} - {...rest} + data-cy={rest['data-cy'] ?? id} + data-tool={rest['data-tool']} > {React.cloneElement(children, { className: classnames(iconSizeClasses[size], 'fill-current'), diff --git a/platform/ui/src/components/SplitButton/SplitButton.tsx b/platform/ui/src/components/SplitButton/SplitButton.tsx index f70c566af..b9d998c94 100644 --- a/platform/ui/src/components/SplitButton/SplitButton.tsx +++ b/platform/ui/src/components/SplitButton/SplitButton.tsx @@ -4,16 +4,17 @@ import classNames from 'classnames'; import OutsideClickHandler from 'react-outside-click-handler'; import { useTranslation } from 'react-i18next'; -import { Icon, Tooltip, ListMenu } from '../'; +import { Icon, Tooltip, ListMenu, ToolbarButton } from '../'; const baseClasses = { Button: - 'flex items-center rounded-md border-transparent border-2 cursor-pointer group/button', + 'flex items-center rounded-md border-transparent cursor-pointer group/button', Primary: - 'h-full flex flex-1 items-center rounded-md rounded-tr-none rounded-br-none group/primary', + // By default border on left, top and bottom for hover effect and only rounded on left side. + // Extra padding on right to componensate for no right border. + 'h-full border-l-2 border-t-2 border-b-2 rounded-tl-md rounded-bl-md group/primary !pl-2 !py-2', Secondary: - 'h-full flex items-center justify-center rounded-tr-md rounded-br-md w-4 group/secondary', - PrimaryIcon: 'w-5 h-5', + 'h-full flex items-center justify-center rounded-tr-md rounded-br-md w-4 border-2 border-transparent group/secondary', SecondaryIcon: 'w-4 h-full stroke-1', Separator: 'border-l py-2.5', Content: 'absolute z-10 top-0 mt-12', @@ -31,29 +32,31 @@ const classes = { Primary: ({ primary, isExpanded }) => classNames( baseClasses.Primary, - primary.isActive && !isExpanded - ? 'bg-primary-light rounded-tr-md rounded-br-md active' - : isExpanded - ? 'bg-primary-dark' - : 'bg-secondary-dark hover:bg-primary-dark' + primary.isActive + ? isExpanded + ? 'border-primary-dark !bg-primary-dark hover:border-primary-dark text-primary-light' + : `${ + primary.isToggle + ? 'border-secondary-dark bg-secondary-light' + : 'border-primary-light bg-primary-light' + } + border-2 rounded-md !p-2` // Full, rounded border with less right padding when active. + : `focus:!text-black focus:!rounded-md focus:!border-primary-light focus:!bg-primary-light + ${ + isExpanded + ? 'border-primary-dark bg-primary-dark !text-primary-light' + : 'border-secondary-dark bg-secondary-dark group-hover/button:border-primary-dark group-hover/button:text-primary-light hover:bg-primary-dark hover:border-primary-dark focus:!text-black' + } + ` ), Secondary: ({ isExpanded, primary }) => classNames( baseClasses.Secondary, isExpanded - ? 'bg-primary-light rounded-tr-md rounded-br-md' + ? 'bg-primary-light !rounded-tr-md !rounded-br-md' : primary.isActive ? 'bg-secondary-dark' - : 'hover:bg-primary-dark bg-secondary-dark' - ), - PrimaryIcon: ({ primary, isExpanded }) => - classNames( - baseClasses.PrimaryIcon, - !primary.isActive && - 'group-hover/primary:text-primary-light group-hover/secondary:text-primary-light group-hover/button:text-primary-light', - primary.isActive && !isExpanded - ? 'text-primary-dark ' - : 'text-common-bright' + : 'hover:bg-primary-dark bg-secondary-dark group-hover/button:border-primary-dark' ), SecondaryIcon: ({ isExpanded }) => classNames( @@ -85,9 +88,12 @@ const SplitButton = ({ items: _items, renderer, onInteraction, + servicesManager, }) => { const { t } = useTranslation('Buttons'); + const { ToolBarService } = servicesManager.services; + const { primaryToolId, toggles } = bState; /* Bubbles up individual item clicks */ const getSplitButtonItems = items => @@ -107,7 +113,7 @@ const SplitButton = ({ setState(state => ({ ...state, - primary: !isAction ? { ...item, index } : state.primary, + primary: !isAction && isRadio ? { ...item, index } : state.primary, isExpanded: false, items: getSplitButtonItems(_items).filter(item => isRadio && !isAction ? item.index !== index : true @@ -133,21 +139,44 @@ const SplitButton = ({ setState(state => ({ ...state, isHovering: false })); const outsideClickHandler = () => setState(state => ({ ...state, isExpanded: false })); - const onPrimaryClickHandler = () => { - onInteraction({ - groupId, - itemId: state.primary.id, - interactionType: state.primary.type, - // splitButtonId? (so we can track group?) - // info to fire item's command/event? - // - commands: state.primary.commands, - }); - }; + const isPrimaryToggle = state.primary.type === 'toggle'; const isPrimaryActive = (state.primary.type === 'tool' && primaryToolId === state.primary.id) || - (state.primary.type === 'toggle' && toggles[state.primary.id] === true); + (isPrimaryToggle && toggles[state.primary.id] === true); + + const PrimaryButtonComponent = + ToolBarService.getButtonComponentForUIType(state.primary.uiType) ?? + ToolbarButton; + + const primaryButtonClassName = classes.Primary({ + ...state, + primary: { isActive: isPrimaryActive, isToggle: isPrimaryToggle }, + }); + + const DefaultListItemRenderer = ({ type, icon, label, t, id }) => { + const isActive = type === 'toggle' && toggles[id] === true; + + return ( +
+ + + + {t(label)} +
+ ); + }; + + const listItemRenderer = renderer || DefaultListItemRenderer; return ( @@ -162,32 +191,20 @@ const SplitButton = ({ onMouseLeave={onMouseLeaveHandler} >
-
- -
- -
- +
+ ToolBarService.recordInteraction(args)} + servicesManager={servicesManager} + // All rounding is taken care of by className + rounded="none" + className={primaryButtonClassName} + data-tool={state.primary.id} + data-cy={`${groupId}-split-button-primary`} + />
renderer({ ...args, t })} + bState={bState} + renderer={args => listItemRenderer({ ...args, t })} />
@@ -234,24 +252,6 @@ const SplitButton = ({ ); }; -const DefaultListItemRenderer = ({ icon, label, isActive, t }) => ( -
- - - - - {t(label)} - -
-); - -const noop = () => {}; - SplitButton.defaultProps = { isRadio: false, isAction: false, @@ -266,7 +266,7 @@ SplitButton.defaultProps = { tooltip: 'More Measure Tools', }, items: [], - renderer: DefaultListItemRenderer, + renderer: null, }; SplitButton.propTypes = { diff --git a/platform/ui/src/components/ToolbarButton/ToolbarButton.tsx b/platform/ui/src/components/ToolbarButton/ToolbarButton.tsx index 76cce19d6..b2fb20da6 100644 --- a/platform/ui/src/components/ToolbarButton/ToolbarButton.tsx +++ b/platform/ui/src/components/ToolbarButton/ToolbarButton.tsx @@ -14,7 +14,9 @@ const ToolbarButton = ({ dropdownContent, // isActive: _isActive, + className, bState = {}, + ...rest // }) => { const { primaryToolId } = bState; @@ -49,7 +51,7 @@ const ToolbarButton = ({ variant={isActive ? 'contained' : 'text'} bgColor={bgClasses[type]} size="toolbar" - className={classnames('mx-1', activeClass, classes[type])} + className={classnames(activeClass, classes[type], className)} onClick={() => { onInteraction({ itemId: id, @@ -60,6 +62,7 @@ const ToolbarButton = ({ name={label} key={id} id={id} + {...rest} >