Skip to content
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

Ensure onHeightChanged is called after DOM change #9670

Open
jryans opened this issue May 10, 2019 · 3 comments
Open

Ensure onHeightChanged is called after DOM change #9670

jryans opened this issue May 10, 2019 · 3 comments
Labels
A-Timeline A-Timeline-Jumpy-Scroll Stable timeline dream ✨ T-Defect X-Needs-Info This issue is blocked awaiting information from the reporter

Comments

@jryans
Copy link
Collaborator

jryans commented May 10, 2019

onHeightChanged is used to notify the scroll panel of dynamic height changes in the timeline.

In a few spots, it looks like we might be calling it too early before the DOM has actually been updated:

This could be a root cause of some scrolling confusion.

@andybalaam
Copy link
Member

@jryans is this still relevant? If we're not actively working on it, I suggest we should add more detail about the problem we think this causes.

@andybalaam andybalaam added X-Needs-Info This issue is blocked awaiting information from the reporter and removed P1 labels Apr 20, 2022
@jryans
Copy link
Collaborator Author

jryans commented Apr 29, 2022

@andybalaam Hmm, I believe the context is some components are calling onHeightChanged before the height of the actual elements have changed. For example, MVideoBody calls it just after setting state to the content URL, but setState is async, so it's unlikely that the video URL has been rendered to the DOM, and thus the video element may still have the wrong height when onHeightChanged.

In any event, I believe this was mainly a theoretical concern I noticed while browsing the code for some other reason. It may be more efficient to close and wait for someone to report the visual issue of scroll jumps, which is how this could manifest if it truly is a problem.

@MadLittleMods MadLittleMods added the A-Timeline-Jumpy-Scroll Stable timeline dream ✨ label May 1, 2023
@MadLittleMods
Copy link
Contributor

Related to #23539

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Timeline A-Timeline-Jumpy-Scroll Stable timeline dream ✨ T-Defect X-Needs-Info This issue is blocked awaiting information from the reporter
Projects
None yet
Development

No branches or pull requests

4 participants