#4469 Improve toolbar ux (#5841)

* Improve ActionButtonDropdown selection

* center rectangle icon fixed

* ignore Esc key when displaying hotkeys

* add ability to escape 3 point circle tool

* remove focus from ActionButton, ActionButtonDropdown

* remove focus outline from buttons

* remember lastly selected multi action item

* Add tests for toolbar buttons

* fix sketch-tests by turning toolbar dropdown arrays into an object with an id - this got broken because dropdown now remember the last selected option so we cant rely on cant reference the first option in tests

* update other tests with open menu click
This commit is contained in:
Andrew Varga
2025-04-09 14:32:52 +02:00
committed by GitHub
parent e78100eaac
commit e5f23a49b4
10 changed files with 743 additions and 621 deletions

View File

@ -137,7 +137,7 @@ async function doBasicSketch(
await page.waitForTimeout(100)
}
await page.getByRole('button', { name: 'Length: open menu' }).click()
await page.getByRole('button', { name: 'constraints: open menu' }).click()
await page.getByRole('button', { name: 'Equal Length' }).click()
// Open the code pane.

View File

@ -1353,4 +1353,51 @@ sketch001 = startSketchOn(XZ)
15
)
})
test(`test-toolbar-buttons`, async ({
page,
homePage,
toolbar,
scene,
cmdBar,
}) => {
await test.step('Load an empty file', async () => {
await page.addInitScript(async () => {
localStorage.setItem('persistCode', '')
})
await page.setBodyDimensions({ width: 1200, height: 500 })
await homePage.goToModelingScene()
// wait until scene is ready to be interacted with
await scene.connectionEstablished()
await scene.settled(cmdBar)
})
await test.step('Test toolbar button correct selection', async () => {
await toolbar.expectToolbarMode.toBe('modeling')
await toolbar.startSketchPlaneSelection()
// Click on a default plane
await page.mouse.click(700, 200)
// tools cannot be selected immediately, couldn't find an event to await instead.
await page.waitForTimeout(1000)
await toolbar.selectCenterRectangle()
await expect(page.getByTestId('center-rectangle')).toHaveAttribute(
'aria-pressed',
'true'
)
})
await test.step('Test Toolbar dropdown remembering last selection', async () => {
// Select another tool
await page.getByTestId('circle-center').click()
// center-rectangle should still be the active option in the rectangle dropdown
await expect(page.getByTestId('center-rectangle')).toBeVisible()
})
})
})

View File

@ -169,7 +169,7 @@ export class ToolbarFixture {
}
selectCenterRectangle = async () => {
await this.page
.getByRole('button', { name: 'caret down Corner rectangle:' })
.getByRole('button', { name: 'caret down rectangles:' })
.click()
await expect(
this.page.getByTestId('dropdown-center-rectangle')
@ -178,7 +178,7 @@ export class ToolbarFixture {
}
selectBoolean = async (operation: 'union' | 'subtract' | 'intersect') => {
await this.page
.getByRole('button', { name: 'caret down Union: open menu' })
.getByRole('button', { name: 'caret down booleans: open menu' })
.click()
const operationTestId = `dropdown-boolean-${operation}`
await expect(this.page.getByTestId(operationTestId)).toBeVisible()
@ -186,25 +186,19 @@ export class ToolbarFixture {
}
selectCircleThreePoint = async () => {
await this.page
.getByRole('button', { name: 'caret down Center circle:' })
.click()
await this.page.getByRole('button', { name: 'caret down circles:' }).click()
await expect(
this.page.getByTestId('dropdown-circle-three-points')
).toBeVisible()
await this.page.getByTestId('dropdown-circle-three-points').click()
}
selectArc = async () => {
await this.page
.getByRole('button', { name: 'caret down Tangential Arc:' })
.click()
await this.page.getByRole('button', { name: 'caret down arcs:' }).click()
await expect(this.page.getByTestId('dropdown-arc')).toBeVisible()
await this.page.getByTestId('dropdown-arc').click()
}
selectThreePointArc = async () => {
await this.page
.getByRole('button', { name: 'caret down Tangential Arc:' })
.click()
await this.page.getByRole('button', { name: 'caret down arcs:' }).click()
await expect(
this.page.getByTestId('dropdown-three-point-arc')
).toBeVisible()

View File

@ -115,7 +115,7 @@ test.describe('Testing constraints', { tag: ['@skipWin'] }, () => {
await page.waitForTimeout(100) // this wait is needed for webkit - not sure why
await page
.getByRole('button', {
name: 'Length: open menu',
name: 'constraints: open menu',
})
.click()
await page.getByRole('button', { name: 'remove constraints' }).click()
@ -189,7 +189,7 @@ test.describe('Testing constraints', { tag: ['@skipWin'] }, () => {
await page.waitForTimeout(100)
await page
.getByRole('button', {
name: 'Length: open menu',
name: 'constraints: open menu',
})
.click()
await page
@ -299,7 +299,7 @@ test.describe('Testing constraints', { tag: ['@skipWin'] }, () => {
await page.keyboard.up('Shift')
await page
.getByRole('button', {
name: 'Length: open menu',
name: 'constraints: open menu',
})
.click()
await page.getByRole('button', { name: constraint }).click()
@ -420,7 +420,7 @@ test.describe('Testing constraints', { tag: ['@skipWin'] }, () => {
await page.waitForTimeout(100)
await page
.getByRole('button', {
name: 'Length: open menu',
name: 'constraints: open menu',
})
.click()
await page
@ -533,7 +533,7 @@ test.describe('Testing constraints', { tag: ['@skipWin'] }, () => {
await page.keyboard.up('Shift')
await page
.getByRole('button', {
name: 'Length: open menu',
name: 'constraints: open menu',
})
.click()
await page.getByTestId('dropdown-constraint-angle').click()
@ -627,7 +627,7 @@ test.describe('Testing constraints', { tag: ['@skipWin'] }, () => {
await page.mouse.click(line3.x, line3.y)
await page
.getByRole('button', {
name: 'Length: open menu',
name: 'constraints: open menu',
})
.click()
await page.getByTestId('dropdown-constraint-' + constraint).click()
@ -719,7 +719,7 @@ part002 = startSketchOn(XZ)
await page.mouse.click(line3.x, line3.y)
await page
.getByRole('button', {
name: 'Length: open menu',
name: 'constraints: open menu',
})
.click()
await page.getByTestId('dropdown-constraint-' + constraint).click()
@ -817,7 +817,7 @@ part002 = startSketchOn(XZ)
const activeLinesContent = await page.locator('.cm-activeLine').all()
const constraintMenuButton = page.getByRole('button', {
name: 'Length: open menu',
name: 'constraints: open menu',
})
const constraintButton = page
.getByRole('button', {
@ -905,7 +905,7 @@ part002 = startSketchOn(XZ)
await page.mouse.click(line3.x - 3, line3.y + 20)
await page.keyboard.up('Shift')
const constraintMenuButton = page.getByRole('button', {
name: 'Length: open menu',
name: 'constraints: open menu',
})
const constraintButton = page.getByRole('button', {
name: constraintName,
@ -990,7 +990,7 @@ part002 = startSketchOn(XZ)
await page.keyboard.up('Shift')
await page.waitForTimeout(100)
const constraintMenuButton = page.getByRole('button', {
name: 'Length: open menu',
name: 'constraints: open menu',
})
const constraintButton = page.getByRole('button', {
name: constraintName,
@ -1057,7 +1057,7 @@ part002 = startSketchOn(XZ)
await page
.getByRole('button', {
name: 'Length: open menu',
name: 'constraints: open menu',
})
.click()
await page.waitForTimeout(500)

View File

@ -124,7 +124,7 @@ test.describe('Testing selections', { tag: ['@skipWin'] }, () => {
// click a segment hold shift and click an axis, see that a relevant constraint is enabled
const constrainButton = page.getByRole('button', {
name: 'Length: open menu',
name: 'constraints: open menu',
})
const absXButton = page.getByRole('button', { name: 'Absolute X' })

View File

@ -17,12 +17,14 @@ import { isDesktop } from '@src/lib/isDesktop'
import { openExternalBrowserIfDesktop } from '@src/lib/openWindow'
import { editorManager, kclManager } from '@src/lib/singletons'
import type {
ToolbarDropdown,
ToolbarItem,
ToolbarItemCallbackProps,
ToolbarItemResolved,
ToolbarItemResolvedDropdown,
ToolbarModeName,
} from '@src/lib/toolbar'
import { toolbarConfig } from '@src/lib/toolbar'
import { isToolbarItemResolvedDropdown, toolbarConfig } from '@src/lib/toolbar'
import { isArray } from '@src/lib/utils'
import { commandBarActor } from '@src/machines/commandBarMachine'
@ -131,21 +133,27 @@ export function Toolbar({
*/
const currentModeItems: (
| ToolbarItemResolved
| ToolbarItemResolved[]
| ToolbarItemResolvedDropdown
| 'break'
)[] = useMemo(() => {
return toolbarConfig[currentMode].items.map((maybeIconConfig) => {
if (maybeIconConfig === 'break') {
return 'break'
} else if (isArray(maybeIconConfig)) {
return maybeIconConfig.map(resolveItemConfig)
} else if (isToolbarDropdown(maybeIconConfig)) {
return {
id: maybeIconConfig.id,
array: maybeIconConfig.array.map((item) =>
resolveItemConfig(item, maybeIconConfig.id)
),
}
} else {
return resolveItemConfig(maybeIconConfig)
}
})
function resolveItemConfig(
maybeIconConfig: ToolbarItem
maybeIconConfig: ToolbarItem,
dropdownId?: string
): ToolbarItemResolved {
const isDisabled =
disableAllButtons ||
@ -176,6 +184,14 @@ export function Toolbar({
}
}, [currentMode, disableAllButtons, configCallbackProps])
// To remember the last selected item in an ActionButtonDropdown
const [lastSelectedMultiActionItem, _] = useState(
new Map<
number /* index in currentModeItems */,
number /* index in maybeIconConfig */
>()
)
return (
<menu
data-current-mode={currentMode}
@ -199,24 +215,33 @@ export function Toolbar({
className="h-5 w-[1px] block bg-chalkboard-30 dark:bg-chalkboard-80"
/>
)
} else if (isArray(maybeIconConfig)) {
} else if (isToolbarItemResolvedDropdown(maybeIconConfig)) {
// A button with a dropdown
const selectedIcon =
maybeIconConfig.array.find((c) => c.isActive) ||
maybeIconConfig.array[lastSelectedMultiActionItem.get(i) ?? 0]
// Save the last selected item in the dropdown
lastSelectedMultiActionItem.set(
i,
maybeIconConfig.array.indexOf(selectedIcon)
)
return (
<ActionButtonDropdown
Element="button"
key={maybeIconConfig[0].id}
data-testid={maybeIconConfig[0].id + '-dropdown'}
id={maybeIconConfig[0].id + '-dropdown'}
name={maybeIconConfig[0].title}
key={selectedIcon.id}
data-testid={selectedIcon.id + '-dropdown'}
id={selectedIcon.id + '-dropdown'}
name={maybeIconConfig.id}
className={
(maybeIconConfig[0].alwaysDark
(maybeIconConfig.array[0].alwaysDark
? 'dark bg-chalkboard-90 '
: '!bg-transparent ') +
'group/wrapper ' +
buttonBorderClassName +
' relative group !gap-0'
}
splitMenuItems={maybeIconConfig.map((itemConfig) => ({
splitMenuItems={maybeIconConfig.array.map((itemConfig) => ({
id: itemConfig.id,
label: itemConfig.title,
hotkey: itemConfig.hotkey,
@ -236,11 +261,11 @@ export function Toolbar({
>
<ActionButton
Element="button"
id={maybeIconConfig[0].id}
data-testid={maybeIconConfig[0].id}
id={selectedIcon.id}
data-testid={selectedIcon.id}
iconStart={{
icon: maybeIconConfig[0].icon,
iconColor: maybeIconConfig[0].iconColor,
icon: selectedIcon.icon,
iconColor: selectedIcon.iconColor,
className: iconClassName,
bgClassName: bgClassName,
}}
@ -248,40 +273,36 @@ export function Toolbar({
'!border-transparent !px-0 pressed:!text-chalkboard-10 pressed:enabled:hovered:!text-chalkboard-10 ' +
buttonBgClassName
}
aria-pressed={maybeIconConfig[0].isActive}
aria-pressed={selectedIcon.isActive}
disabled={
disableAllButtons ||
maybeIconConfig[0].status !== 'available' ||
maybeIconConfig[0].disabled
selectedIcon.status !== 'available' ||
selectedIcon.disabled
}
name={maybeIconConfig[0].title}
name={selectedIcon.title}
// aria-description is still in ARIA 1.3 draft.
// eslint-disable-next-line jsx-a11y/aria-props
aria-description={maybeIconConfig[0].description}
onClick={() =>
maybeIconConfig[0].onClick(configCallbackProps)
}
aria-description={selectedIcon.description}
onClick={() => selectedIcon.onClick(configCallbackProps)}
>
<span
className={!maybeIconConfig[0].showTitle ? 'sr-only' : ''}
>
{maybeIconConfig[0].title}
<span className={!selectedIcon.showTitle ? 'sr-only' : ''}>
{selectedIcon.title}
</span>
<ToolbarItemTooltip
itemConfig={maybeIconConfig[0]}
itemConfig={selectedIcon}
configCallbackProps={configCallbackProps}
wrapperClassName="ui-open:!hidden"
contentClassName={tooltipContentClassName}
>
{showRichContent ? (
<ToolbarItemTooltipRichContent
itemConfig={maybeIconConfig[0]}
itemConfig={selectedIcon}
/>
) : (
<ToolbarItemTooltipShortContent
status={maybeIconConfig[0].status}
title={maybeIconConfig[0].title}
hotkey={maybeIconConfig[0].hotkey}
status={selectedIcon.status}
title={selectedIcon.title}
hotkey={selectedIcon.hotkey}
/>
)}
</ToolbarItemTooltip>
@ -430,7 +451,9 @@ const ToolbarItemTooltipShortContent = ({
>
{title}
{hotkey && (
<kbd className="inline-block ml-2 flex-none hotkey">{hotkey}</kbd>
<kbd className="inline-block ml-2 flex-none hotkey">
{displayHotkeys(hotkey)}
</kbd>
)}
</span>
)
@ -461,7 +484,9 @@ const ToolbarItemTooltipRichContent = ({
{itemConfig.title}
</span>
{itemConfig.status === 'available' && itemConfig.hotkey ? (
<kbd className="flex-none hotkey">{itemConfig.hotkey}</kbd>
<kbd className="flex-none hotkey">
{displayHotkeys(itemConfig.hotkey)}
</kbd>
) : itemConfig.status === 'kcl-only' ? (
<>
<span className="text-wrap font-sans flex-0 text-chalkboard-70 dark:text-chalkboard-40">
@ -522,3 +547,14 @@ const ToolbarItemTooltipRichContent = ({
</>
)
}
// We don't want to display Esc hotkeys to avoid confusion in the Toolbar UI (eg. "EscR")
function displayHotkeys(hotkey: string | string[]) {
return (isArray(hotkey) ? hotkey : [hotkey]).filter((h) => h !== 'Esc')
}
function isToolbarDropdown(
item: ToolbarItem | ToolbarDropdown
): item is ToolbarDropdown {
return 'array' in item
}

View File

@ -71,6 +71,7 @@ export const ActionButton = forwardRef((props: ActionButtonProps, ref) => {
<button
ref={ref as ForwardedRef<HTMLButtonElement>}
className={classNames}
tabIndex={-1}
{...rest}
>
{iconStart && <ActionIcon {...iconStart} />}

View File

@ -69,6 +69,7 @@ export function ActionButtonDropdown({
close()
}}
className="group/button flex items-center gap-6 px-3 py-1 font-sans text-xs hover:bg-primary/10 dark:hover:bg-chalkboard-80 border-0 m-0 w-full rounded-none text-left disabled:!bg-transparent dark:disabled:text-chalkboard-60"
tabIndex={-1}
disabled={item.disabled}
data-testid={'dropdown-' + item.id}
>

View File

@ -86,7 +86,7 @@ textarea,
button {
@apply border border-chalkboard-30 m-0.5 px-3 rounded text-xs;
@apply focus-visible:outline-chalkboard-100;
@apply focus-visible:outline-none;
}
button:hover {
@ -94,7 +94,7 @@ button:hover {
}
.dark button {
@apply border-chalkboard-70 focus-visible:outline-chalkboard-10;
@apply border-chalkboard-70;
}
.dark button:hover {

View File

@ -15,7 +15,12 @@ export type ToolbarModeName = 'modeling' | 'sketching'
type ToolbarMode = {
check: (state: StateFrom<typeof modelingMachine>) => boolean
items: (ToolbarItem | ToolbarItem[] | 'break')[]
items: (ToolbarItem | ToolbarDropdown | 'break')[]
}
export type ToolbarDropdown = {
id: string
array: ToolbarItem[]
}
export interface ToolbarItemCallbackProps {
@ -58,6 +63,17 @@ export type ToolbarItemResolved = Omit<
isActive?: boolean
}
export type ToolbarItemResolvedDropdown = {
id: string
array: ToolbarItemResolved[]
}
export const isToolbarItemResolvedDropdown = (
item: ToolbarItemResolved | ToolbarItemResolvedDropdown
): item is ToolbarItemResolvedDropdown => {
return (item as ToolbarItemResolvedDropdown).array !== undefined
}
export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
modeling: {
check: (state) =>
@ -208,7 +224,9 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
links: [{ label: 'KCL docs', url: 'https://zoo.dev/docs/kcl/shell' }],
},
'break',
[
{
id: 'booleans',
array: [
{
id: 'boolean-union',
onClick: () =>
@ -267,8 +285,11 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
],
},
],
},
'break',
[
{
id: 'planes',
array: [
{
id: 'plane-offset',
onClick: () => {
@ -299,6 +320,7 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
links: [],
},
],
},
{
id: 'helix',
onClick: () => {
@ -315,7 +337,9 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
links: [{ label: 'KCL docs', url: 'https://zoo.dev/docs/kcl/helix' }],
},
'break',
[
{
id: 'ai',
array: [
{
id: 'text-to-cad',
onClick: () =>
@ -352,6 +376,7 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
links: [],
},
],
},
],
},
sketching: {
@ -403,7 +428,9 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
links: [],
isActive: (state) => state.matches({ Sketch: 'Line tool' }),
},
[
{
id: 'arcs',
array: [
{
id: 'tangential-arc',
onClick: ({ modelingState, modelingSend }) =>
@ -428,7 +455,9 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
: undefined,
title: 'Tangential Arc',
hotkey: (state) =>
state.matches({ Sketch: 'Tangential arc to' }) ? ['Esc', 'A'] : 'A',
state.matches({ Sketch: 'Tangential arc to' })
? ['Esc', 'A']
: 'A',
description: 'Start drawing an arc tangent to the current segment',
links: [],
isActive: (state) => state.matches({ Sketch: 'Tangential arc to' }),
@ -439,7 +468,9 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
modelingSend({
type: 'change tool',
data: {
tool: !modelingState.matches({ Sketch: 'Arc three point tool' })
tool: !modelingState.matches({
Sketch: 'Arc three point tool',
})
? 'arcThreePoint'
: 'none',
},
@ -481,6 +512,7 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
isActive: (state) => state.matches({ Sketch: 'Arc tool' }),
},
],
},
{
id: 'spline',
onClick: () => console.error('Spline not yet implemented'),
@ -492,7 +524,9 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
links: [],
},
'break',
[
{
id: 'circles',
array: [
{
id: 'circle-center',
onClick: ({ modelingState, modelingSend }) =>
@ -508,9 +542,7 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
status: 'available',
title: 'Center circle',
disabled: (state) => state.matches('Sketch no face'),
isActive: (state) =>
state.matches({ Sketch: 'Circle tool' }) ||
state.matches({ Sketch: 'Circle three point tool' }),
isActive: (state) => state.matches({ Sketch: 'Circle tool' }),
hotkey: (state) =>
state.matches({ Sketch: 'Circle tool' }) ? ['Esc', 'C'] : 'C',
showTitle: false,
@ -533,12 +565,19 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
icon: 'circle',
status: 'available',
title: '3-point circle',
isActive: (state) =>
state.matches({ Sketch: 'Circle three point tool' }),
hotkey: (state) =>
state.matches({ Sketch: 'Circle three point tool' }) ? 'Esc' : [],
showTitle: false,
description: 'Draw a circle defined by three points',
links: [],
},
],
[
},
{
id: 'rectangles',
array: [
{
id: 'corner-rectangle',
onClick: ({ modelingState, modelingSend }) =>
@ -573,7 +612,7 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
: 'none',
},
}),
icon: 'arc',
icon: 'rectangle',
status: 'available',
disabled: (state) => state.matches('Sketch no face'),
title: 'Center rectangle',
@ -588,6 +627,7 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
},
},
],
},
{
id: 'polygon',
onClick: () => console.error('Polygon not yet implemented'),
@ -619,7 +659,9 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
description: 'Mirror sketch entities about a line or axis',
links: [],
},
[
{
id: 'constraints',
array: [
{
id: 'constraint-length',
disabled: (state) =>
@ -882,6 +924,7 @@ export const toolbarConfig: Record<ToolbarModeName, ToolbarMode> = {
links: [],
},
],
},
],
},
}