Skip to content

fix: address review follow-ups on the canvas stack - #150

Open
librowski wants to merge 1 commit into
ds2-tokens-refresh-0915from
ds2-review-follow-ups
Open

librowski wants to merge 1 commit into
ds2-tokens-refresh-0915from
ds2-review-follow-ups

Conversation

@librowski

Copy link
Copy Markdown
Collaborator

Summary

Follow-ups from the second review round on the canvas stack (#124, #126, #136, #139, #140). Those PRs are approved, so the fixes land here on top of #149 instead of being force-pushed into the reviewed branches.

Changes

Persist without measured (#124)

  • getNodesWithoutDynamicValues also strips the runtime dragging flag; the bundled Demo and AI Studio templates no longer carry dragging: false.
  • getStoreDataForIntegration gets two boundary tests: default output has no measured, dragging or selection and empty routing points; shouldSkipDynamicValues: false returns the live store arrays.
  • Changeset mentions dragging.

Node text ellipsis (#126)

  • Docs and changeset state the contain: inline-size constraint: the block needs an ancestor with a definite inline size and collapses to zero width inside inline-flex or fit-content wrappers.

Node body spacing (#136)

  • Width decision log rewritten with the current derivation (inset 8 + 1, 205px, body and row roles from the export) and a ## Status section; the dated update sections and the external tracker reference are gone.
  • Tooltip changeset reworded.

Menu item selected (#139)

  • Base UI Menu.RadioGroup renders its own div[role="group"], which swallowed the popup's column gap. The wrapper now gets .group from list-box.module.css (column flex, --wb-public-list-box-gap). Measured in Demo: 4px between radio items, same as plain items.
  • menu.spec.tsx and node-panel.spec.tsx unmount the React root after each test.

Palette node states (#140)

  • The custom node guide and the Demo multi-port template forward disabled to NodePanel.Root, NodeIcon and NodeDescription; the guide explains when the palette passes selected and disabled.
  • node-panel.mdx lists disabled?: boolean on NodePanel.Root.
  • Changeset is written around WorkflowNodeTemplateProps.disabled with a migration note for custom templates.

Not changed

Checks

  • pnpm -F @workflowbuilder/ui build (includes check:built-css), vitest ui 74 and sdk 218 passing, stylelint ui/sdk clean, typecheck ui/sdk/demo/ai-studio/docs clean.

- persist: strip the runtime dragging flag alongside measured and selected,
  drop dragging from the bundled templates, cover getStoreDataForIntegration
- node description: document the contain: inline-size constraint
- connectable item: rewrite the width decision log with the current numbers
  and a status section; reword the tooltip changeset
- menu: lay out the RadioGroup wrapper as a column so the list-box gap applies
- ui specs: unmount the React root after each test
- palette: forward disabled in the custom template guide and the demo
  multi-port template, document it on NodePanel.Root, reword the changeset
<NodePanel.Root selected={selected}>
<NodePanel.Root selected={selected} disabled={disabled}>
<NodePanel.Header>
<NodeIcon icon={iconElement} />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The guide snippet and the changeset's migration note forward disabled to all three parts, but here only NodePanel.Root gets it. NodeIcon and NodeDescription mute only through their own prop, and the shell's disabled swaps just the background, so the read-only palette shows this entry with a faded surface but full-contrast icon and text. Could we pass disabled={disabled} on lines 46-47 as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants