-
Notifications
You must be signed in to change notification settings - Fork 110
H-5976: Fix display of tooltips for readonly inputs #8285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
2ede0e8 to
c89a1ba
Compare
PR SummaryMedium Risk Overview Introduces reusable form controls ( Tightens read-only enforcement by extending Written by Cursor Bugbot for commit 99fdc4d. This will update automatically on new commits. Configure here. |
🤖 Augment PR SummarySummary: This PR improves read-only UX during Petrinaut simulation by ensuring disabled/readonly controls can still display explanatory tooltips. Changes:
Technical Notes: Read-only state is now centralized via 🤖 Was this summary useful? React with 👍 or 👎 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libs/@hashintel/petrinaut/src/views/Editor/panels/PropertiesPanel/parameter-properties.tsx
Outdated
Show resolved
Hide resolved
Using inline-flex caused block-level children like CodeEditor to lose their width. Using flex ensures the wrapper takes full width while still properly wrapping both block and inline elements. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Ensure the flex wrapper takes full width of its parent container so block-level children like CodeEditor don't collapse. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Block-level div elements properly contain block-level children like CodeEditor. The span element wasn't correctly handling the width inheritance for Monaco Editor containers. Co-Authored-By: Claude Opus 4.5 <[email protected]>
The previous fix using flex with width: 100% was breaking the dimensions of input elements. This change uses inline-flex instead, which preserves the natural dimensions of input elements while still enabling tooltips on disabled elements. The inline-flex display mode ensures that: 1. Input elements maintain their intended width 2. Tooltips still work on disabled elements 3. The wrapper doesn't force unwanted width constraints 4. Alignment is preserved with align-items: center Fixes issue with Tooltip wrapper breaking dimensions of inputs.
- Tooltip now accepts a `display` prop ("block" | "inline") to control
wrapper element behavior, preventing layout issues
- Block mode (default): For full-width elements like inputs/selects
- Inline mode: For buttons and inline elements in flex containers
- Simplified SegmentGroup and Switch components to use shared Tooltip
- CodeEditor now shows tooltip on edit attempts in readonly mode via
Monaco's onDidAttemptReadOnlyEdit event
- Added missing `display="inline"` to button tooltips throughout the app
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
Introduces a reusable `withTooltip` higher-order component that adds `tooltip` and `tooltipDisplay` props to any component. Refactors SegmentGroup and Switch to use this HoC instead of inline tooltip handling. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Create Input, NumberInput, Slider, Button, IconButton, and Select components - All components use withTooltip HoC for automatic tooltip support - Replace native form elements with new components across: - parameter-properties.tsx - transition-properties.tsx - place-properties.tsx - sortable-arc-item.tsx - type-properties.tsx - differential-equation-properties.tsx - place-initial-state.tsx - Remove redundant inline style definitions (inputStyle, deleteButtonStyle, etc.) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Use type aliases instead of interfaces for component props to follow project conventions. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix memory leak by adding cleanup useEffect for timeout - Fix edit listener not registering when readOnly changes dynamically by storing editor ref and using useEffect to manage listener lifecycle - Fix tooltip not showing on hover in read-only mode by combining isHovering and showReadOnlyTooltip states - Rename 'editor' variable to 'editorElement' to avoid shadowing - Add display="inline" to AI menu tooltip in transition-properties Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix CodeEditor duplicate listener by using isEditorMounted state to trigger useEffect after Monaco mounts, removing registration from handleMount callback - Add tooltip prop to SortableArcItem for read-only mode feedback - Pass read-only tooltip to Input/Output Arc weight inputs Co-Authored-By: Claude Opus 4.5 <[email protected]>
2eb875f to
1f85b90
Compare
libs/@hashintel/petrinaut/src/views/Editor/subviews/place-initial-state.tsx
Show resolved
Hide resolved
lunelson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parts related to Tooltip seem okay to me 👍🏼
CiaranMn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost everything looks good and tooltips work well – a couple of issues in the DE section:
- If a type is not selected, the dropdown doesn't appear correctly (in readonly mode it's the same but also has no width in addition to no height)
- In readonly mode, the code editor does not show
- If populated, the dropdown appearance (with type selected) has changed in a different way in readonly and non-readonly mode
a. in readonly, it is content width only
b. in not readonly it is full width and centre justified.
Previously, it was always full width and left justified.
- Add placeholder text and empty state for type dropdown in differential equation properties - Simplify CodeEditor tooltip by using standard Tooltip component with className support - Add className prop to Tooltip component using cx helper - Update button styles with disabled variant styling for type properties - Add read-only tooltip to place initial state number input Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| /> | ||
| ); | ||
|
|
||
| export const Slider = withTooltip(SliderBase, "inline"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused Slider component is dead code
Low Severity
The entire Slider component file is dead code. While the PR description lists it as a "New Reusable Form Component," it is never imported or used anywhere in the codebase. There's also an existing sliderStyle in simulation-controls.tsx with nearly identical styling that doesn't use this new component. This unused component adds maintenance burden without providing value.

🌟 What is the purpose of this PR?
Improve UX in simulation/read-only mode by ensuring disabled/readonly controls display explanatory tooltips when hovered.
Define previously native components, as custom components in
src/componentsto prepare future migration to the Design System.Define
withTooltipHoC as discussed with @lunelson to have a "standardized" way to compose Tooltips with other components. (Components from the Design System won't be aware of tooltips)🔗 Related links
🚫 Blocked by
Nothing
🔍 What does this change?
Tooltip Component Refactoring
displayprop with two variants:"block"(default): For full-width elements like inputs and selects"inline": For buttons and inline elements in flex containerswithTooltipHoC to reduce boilerplate when adding tooltip support to componentsNew Reusable Form Components
All components use the
withTooltipHoC for automatic tooltip support:monospaceandhasErrorvariantsdefaultandghostvariantssm/md/lg) and variant (default/danger) optionsCodeEditor Component
onDidAttemptReadOnlyEditlistenerisEditorMountedstate to properly register listeners after Monaco mountsProperties Panel Refactoring
Replaced native HTML form elements with new reusable components across all properties panels:
parameter-properties.tsxtransition-properties.tsxplace-properties.tsxsortable-arc-item.tsx- Added tooltip support for arc weight inputstype-properties.tsxdifferential-equation-properties.tsxplace-initial-state.tsxThis removes redundant inline style definitions and ensures consistent tooltip behavior.
Component Updates
withTooltipHoC with disabled state stylingwithTooltipHoC for consistent tooltip behaviorRead-Only Mode Enforcement
useIsReadOnlyhook now treatsCompletesimulation state as read-onlyNew UI Messages
DYNAMICS_REQUIRES_TYPEmessage for the Dynamics toggle when no type is selectedPre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
None
🐾 Next steps
disabledvsaria-disabledThis PR adds a span inside
Tooltipcomponent.Not the best solution, but works with
disabled: trueon the child component.One possible next step is to remove usage of
disabledand rely instead onaria-disabled, which will remove the need for a wrapper.🛡 What tests cover this?
❓ How to test this?
yarn dev📹 Demo
N/A