[ENG-1545] Switch positions of node type and node title fields#907
[ENG-1545] Switch positions of node type and node title fields#907trangdoan982 wants to merge 7 commits intomainfrom
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Move Content input above Node Type selector so users can immediately start typing. Support empty Node Type (queries all node types), auto-detect type on selection, and lock type when a node is selected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
trangdoan982
force-pushed
the
worktree-eng-1545-switch-positions-of-node-type-and-node-title-fields
branch
from
43e24d0 to
91cec86
Compare
| style={{ pointerEvents: "all" }} | ||
| > | ||
| <div className={`${Classes.DIALOG_BODY} flex flex-col gap-4`}> | ||
| {/* Content Input */} |
There was a problem hiding this comment.
| renderModifyNodeDialog({ | ||
| mode: "create", | ||
| nodeType: defaultNodeType, | ||
| nodeType: "", |
There was a problem hiding this comment.
|
|
||
| const [selectedNodeType, setSelectedNodeType] = useState(() => { | ||
| const [selectedNodeType, setSelectedNodeType] = useState< | ||
| (typeof discourseNodes)[number] | null |
There was a problem hiding this comment.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if (!selectedNodeType && r.uid) { | ||
| const detectedType = (r as Record<string, unknown>) | ||
| .discourseNodeType as string | undefined; | ||
| if (detectedType) { |
There was a problem hiding this comment.
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Thank you for adding inline comments, but I'm not clear if this PR is solving all tickets, or these are intended to be some placeholder code. Unfortunately the YouTube video was private, so I'm not sure if that question was answered there.
If this PR is solving all tickets, make sure to link the PR to the Linear tickets.
This biggest change request is querying for all nodes when no node type is selected.
|
@mdroidian this PR addresses all of the sub-tickets and the inline comments meant to point out the section that work on each sub-ticket. My bad I also made the video unlisted so you can see again. Same behavior except the focus now shows for the node type menu when user focuses there 
|
Ok. If this PR is ready for review again, please tag me as a reviewer/re-request review. |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Generally speaking we want to avoid !—it bypasses null safety.
| ? "cursor-not-allowed opacity-50" | ||
| : "", | ||
| onFocus: (e) => { | ||
| e.currentTarget.style.boxShadow = `0 0 0 2px ${Colors.BLUE3}, 0 0 0 4px ${Colors.BLUE3}4d`; |
There was a problem hiding this comment.
Could you elaborate on why we are adding custom styling to this button? Should other buttons in dg follow this pattern? What problem is this solving for "[ENG-1545] Switch positions of node type and node title fields"?
There was a problem hiding this comment.
this change was to addressed your previous comment about the focus state for node type menu not showing.
This is generally because of how MenuItemSelect doesn't show focus status on the button (Query builder also has this issue). If we try setting it using className, it's added to the Select component and not the Button component. That's why I needed to add the style for onFocus in the ButtonProps. this style matches with Blueprint's CSS class .bp3-button:focus
2 participants
https://youtu.be/X0MbEH5GjvU
Summary
ENG-1316 regression safety
All focus fixes preserved:
autoFocus={!isContentLocked}on Content,autoFocus={false}on Node Type Label,popoverProps={{ openOnTargetFocus: false }}, keyup handler in FuzzySelectInput, and confirm buttonautoFocus={isContentLocked}with key remount.Test plan
🤖 Generated with Claude Code