-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Update getCurrentOutputInfo() on element resize or visibility change #3682
base: main
Are you sure you want to change the base?
Conversation
…ing's container size changes (instead of only when the window resizes)
…ntersectionObserver to send size/hidden state
…display:none on output children when resizing
e8f7f80
to
f9c0d46
Compare
This comment was marked as outdated.
This comment was marked as outdated.
@cpsievert what does a "stretchy output container" look like? e.g. what's an example app that doesn't resize the image properly without that fbe9d24 code? (Want to make sure my PR #3683 isn't overlooking an edge case, and that it's compatible with this fix.) |
function doSendOutputHiddenState() { | ||
const visibleOutputs = {}; | ||
io.observe(el); | ||
$(el).data("shiny-intersection-observer", io); |
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.
Double-check on the UX with scrolling here
Currently shiny.js only sends input values tied to output values (e.g.,
.clientdata_output_{id}_width
, etc) to the server (i.e.,getCurrentOutputInfo()
) when the window resizes (or if certain Bootstrap events happens). This PR leveragesResizeObserver()
andIntersectionObserver()
to update those relevant input values whenever the element is resized or has its visibility changed (regardless of what has caused that change)TODO
Investigate more why fbe9d24 is needed(it's needed because, if the output container is stretchy, the image won't know to shrink when the container shrinks since it's size is determined by the parent)