-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
plot resize with draggable panel #352
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Works very smoothly too.
We should discuss integration into SAM floaters next. Not trivial!
styleOverrides={{ | ||
resize: 'both', | ||
}} | ||
onPanelResize={(dimensions: HeightAndWidthInPixels) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to use useCallback
on the function here and put the dependency back (see below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or even just onPanelResize={setPanelDimension}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useState
setters are guaranteed to be stable
width: width, | ||
}); | ||
}, | ||
[height, width, onPanelResize] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be reinstated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bobular By having onPanelResize, it causes infinite loop, thus I removed it in the dependencies, only to use [height, width] dependencies in the next line: Line 117.
Do we need to add
Do all the floaters have a "main plot" - the contingency table might not. Can't test right now due to back end problems. |
I think maybe we only let the draggable size control the width of the plot, and we set the height according to the aspect ratio of the default size? We probably need to play with it a bit. |
First of all thank you for your feedbacks above, @bobular 👍 As for this, actually that's what I wanted to ask/suggest when I alluded at Slack (😅) , because it is arguably the simplest solution without bothering the other components in a floating Viz such as input variable selector at top, plot controls and bottom, etc. |
This is a test for resizing plotly's plot embedded in a draggable (and resizable) panel, like used in SAM.
Tested several ways to make it happen but the only way I could do was to use draggable panel's onPanelResize() that returns current size of the panel. Then, a plot size in which a barplot is utilized for test purpose is determined by the current panel size. Please note that current format has an issue for the actual panel size, thus it is modified to avoid infinite loop of plot resize.
Here is a demo video:
plot.resize.with.draggable.panel.mp4