feat: add CanvasContext #4439
Open
feat: add CanvasContext #4439
+215
−105
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
plagoa
requested review from
zettca and
MEsteves22
and removed request for
a team
plagoa
force-pushed
the
feat/canvas_context
branch
2 times, most recently
from
dd86921
to
7b5fe70
Compare
zettca
reviewed
Nov 19, 2024
| () => ({ | ||
| sidePanelOpen, | ||
| updateSidePanelOpen, | ||
| sidePanelWidth: width, |
There was a problem hiding this comment.
Suggested change
| sidePanelWidth: width, | |
| sidePanelWidth: sidePanelOpen ? width : 0, |
| @@ -1,5 +1,5 @@ | |||
| import { useMemo, useState } from "react"; | |||
| import { cx } from "@emotion/css"; | |||
| import HvCanvasProvider from "packages/pentaho/src/Canvas/CanvasContext/CanvasContext"; | |||
There was a problem hiding this comment.
👀
needs to be exported
Can we hoist CanvasContext.tsx up a directory too? We won't have other files there, and its how we organize other contexts
| const HvCanvasProvider = ({ children }: { children: React.ReactNode }) => { | ||
| const [sidePanelOpen, setSidePanelOpen] = useState(false); | ||
| const [width, setWidth] = useState(0); | ||
| const [lastOpenWidth, setLastOpenWidth] = useState(0); |
There was a problem hiding this comment.
This is derived state, right? We can compute the actual width with the existing sidePanelOpen and width
eg
const actualWidth = sidePanelOpen ? width : 0| @@ -19,6 +19,7 @@ interface ResizableProps { | |||
| initialWidth?: number; | |||
| minWidth?: number; | |||
| maxWidth?: number; | |||
| updateSidePanelWidth?: (width: number) => void; | |||
There was a problem hiding this comment.
event handlers should be named as such on*. eg
Suggested change
| updateSidePanelWidth?: (width: number) => void; | |
| onResize?: (width: number) => void; |
| ref={ref} | ||
| className={cx(classes.root, className)} | ||
| style={{ | ||
| width: sidePanelOpen |
There was a problem hiding this comment.
we could do this with CSS vars
Suggested change
| width: sidePanelOpen | |
| "--sizePanelWidth": open ? sidePanelWidth : 0, |
plagoa
force-pushed
the
feat/canvas_context
branch
from
7b5fe70
to
fa6c1dc
Compare
plagoa
force-pushed
the
feat/canvas_context
branch
from
fa6c1dc
to
4a97967
Compare
zettca
reviewed
Nov 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a
HvCanvasContext. Canvas components (SidePanel,BottomPanelandCanvasToolbar) use this context to resize automatically when the side panel is resized. If no CanvasContext is used then the components won't resize automatically and it's up to the user to do that if that's needed.