feat(frontend): canvas overlay refactor - #599
Conversation
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
jaskfla
left a comment
jaskfla
left a comment
There was a problem hiding this comment.
Big question is: is context the right tool?
| topLeftCoordinates: Point; | ||
| setImageOverlay: (overlay: ImageOverlayState | null) => void; | ||
| setShowOverlay: (show: boolean) => void; | ||
| setTopLeftCoordinates: (point: Point) => void; |
There was a problem hiding this comment.
| setTopLeftCoordinates: (point: Point) => void; | |
| setOrigin: (point: Point) => void; |
| inset: 0, | ||
| pointerEvents: "none", | ||
| position: "absolute", | ||
| zIndex: 1, |
There was a problem hiding this comment.
Is z-index necessary? Can the node be rendered later in the tree instead?
| } | ||
|
|
||
| setImageOverlay({ | ||
| alt: "Preview of uploaded image", |
There was a problem hiding this comment.
Can probably remove this; it’s not adding any meaning for a screen reader user
|
|
||
| interface ImageOverlayContextType { | ||
| imageOverlay: ImageOverlayState | null; | ||
| showOverlay: boolean; |
There was a problem hiding this comment.
Is showOverlay is doing anything that can’t be derived from imageOverlay? Looks like showOverlay is always equivalent to imageOverlay === null by my read
| null, | ||
| ); | ||
| const { | ||
| setImageOverlay, |
There was a problem hiding this comment.
This control feels inverted. Currently: setImageOverlay is being called by a child of ImageOverlayContext. This causes a render loop that settles, which is one technical consideration; but it’s secondary to the responsibility maybe being backward
There’s only one consumer, so I’m not quite sure (admittedly from just reading the code) that it should be a context.
Tell me if this is impractical, but imageOverlay should be defined in the same component that renders ImageOverlayContext (big finger quotes around “context”), whose key should be tied to imageOverlay—and that sounds suspiciously like regular state management, not a context 🤔
3 participants
No description provided.