Skip to content

#10257 Confirmation dialogue button alignment#10267

Open
andrewvarga wants to merge 7 commits intomainfrom
andrewvarga/10257/confirmation-dialogue-button-alignment
Open

#10257 Confirmation dialogue button alignment#10267
andrewvarga wants to merge 7 commits intomainfrom
andrewvarga/10257/confirmation-dialogue-button-alignment

Conversation

@andrewvarga
Copy link
Contributor

Fixes #10257 and a few related issues.

DeleteProjectDialog.tsx previously:
Screenshot 2026-03-03 at 11 51 13

Now (on Windows the order is: Delete, Cancel but they are still both aligned to the right):
Screenshot 2026-03-03 at 11 51 01

SetVarNameModal.tsx previously:

Screenshot 2026-03-03 at 11 09 01

Now:
Screenshot 2026-03-03 at 11 16 39

ToastUpdate.tsx previously:
Screenshot 2026-03-03 at 11 21 14

Now:
Screenshot 2026-03-03 at 11 38 37

  • Removed UpdatedRestartModal.tsx because it wasn't used anywhere, I think this was replaced by ToastUpdate.tsx
  • Fixed a React warning in DeleteProjectDialog.tsx:
ProjectCard.tsx:219 In HTML, <p> cannot be a descendant of <p>.
This will cause a hydration error.
  ...
    <j3 target={{current:null}}>
      <l6 force={false}>
        <z initialFocus={undefined} containers={function} features={22}>
          <l4>
          <div onKeyDown={function onKeyDown} onBlur={function onBlur} ref={function}>
            <div className="relative z-50" id="headlessui..." role="dialog" aria-modal={true} aria-labelledby={null} ...>

@andrewvarga andrewvarga requested a review from a team as a code owner March 3, 2026 11:58
@vercel
Copy link

vercel bot commented Mar 3, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
modeling-app Ready Ready Preview, Comment Mar 9, 2026 0:31am

Request Review

Comment on lines +19 to +32
useEffect(() => {
const onWindowKeyDown = (event: KeyboardEvent) => {
if (event.key === 'Escape') {
event.preventDefault()
event.stopPropagation()
onDismiss()
}
}

window.addEventListener('keydown', onWindowKeyDown)
return () => {
window.removeEventListener('keydown', onWindowKeyDown)
}
}, [onDismiss])
Copy link
Contributor

Choose a reason for hiding this comment

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

The custom Escape key handler is redundant and potentially problematic. The @headlessui/react Dialog component already handles Escape key presses via the onClose prop (line 61). This custom useEffect hook adds a window-level listener that will also call onDismiss() when Escape is pressed, potentially causing onDismiss to be invoked twice.

The event.stopPropagation() may not prevent the Dialog's internal handler from firing since they operate at different levels.

Fix: Remove this entire useEffect block. The Dialog's built-in Escape handling is sufficient:

// Remove lines 19-32
// The Dialog component's onClose={onDismiss} already handles Escape

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Copy link
Contributor

@jtran jtran left a comment

Choose a reason for hiding this comment

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

Code looks good to me, and I tested the delete project. I don't know if the graphite comment is right, though.

@andrewvarga
Copy link
Contributor Author

Code looks good to me, and I tested the delete project. I don't know if the graphite comment is right, though.

Yeah, I tried with the Dialog's onClose first but that never triggered because we're capturing key presses in other places too..

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.

On mac, the delete project dialog buttons are backwards

2 participants