Repository navigation
Improvements to interactive modal component - #289
LaurenAllin wants to merge 5 commits into
Conversation
…een modal and buttons.
…ut from mouse, trackpad, stylus, and mobile screens.
|
This caller still passes |
|
@LaurenAllin I guess there were more comments than I initially thought, but pretty minor changes. You'll want to rebase against main so you have the most recent changes from that branch to keep this one clean. Let me know if you need any help getting that to work. |
| <div className="gw-fixed gw-inset-0 gw-w-screen gw-overflow-auto gw-p-4"> | ||
| <div className="gw-flex gw-min-h-full gw-items-center gw-justify-center"> | ||
| <div | ||
| style={ |
There was a problem hiding this comment.
This style expression uses the comma operator, so the first object with background, padding, overflow, borderRadius, and zIndex is discarded. The wrapper only receives the conditional width/height styles. This should be one merged style object.
There was a problem hiding this comment.
or should use expansion on the objects so that the results of the conditional are merged with the initial object
| unmount={unmount} | ||
| role={role} | ||
| className={gwMerge("gw-relative", "gw-z-[200]", className)} | ||
| style={{ |
There was a problem hiding this comment.
The new background prop is applied to the Dialog, but the actual DialogBackdrop still has a hard-coded gw-bg-black/30. That means consumers cannot really control the overlay color. The prop should be applied to the backdrop or the hard-coded backdrop class should be conditional.
| width: widthClass, | ||
| height: null, | ||
| }); | ||
| const [position, setPosition] = useState({ x: 0, y: 0 }); //May not be needed |
There was a problem hiding this comment.
I think this should be removed, looks to be unused
| window.removeEventListener("pointercancel", handleRelease); | ||
| }; | ||
| // Ensure all values read by handleMove are in the dependency array to prevent stale closures | ||
| }, [resizing, offset, windowWidth, windowHeight]); |
There was a problem hiding this comment.
handleMove is not in the dependency array, throwing an eslint warning.
This branch has not been deployed
2 participants
Summary
Checklist
Release impact