feat(span-tree): Horizontal autoscrolling on the span tree#34706
Conversation
|
Please approve this PR for the feature flag as well :) |
|
Okay, no comments on the code yet, it mostly looks fine, but I played around with it. It still has a somewhat "boaty" feel, and I noticed depending on how fast you scroll you get inconsistencies (I'll dm you a recording). Two things:
|
size-limit report 📦
|
k-fish
left a comment
k-fish
left a comment
There was a problem hiding this comment.
Nice! I pulled and tried locally and aside from the small bug I dm'd you, I think this is good enough to try out. No need to over-optimize, we can tune it more later or if we get feedback.
| return; | ||
| } | ||
|
|
||
| const left = this.spansInView.getScrollVal(); |
There was a problem hiding this comment.
I think you might want to calc left inside the performScroll since it would be the most up to date there? It might not matter functionally though, it does save calling it twice.
There was a problem hiding this comment.
Hmmm, this makes sense, but performScroll would have to be restructured since it's also being used for the manual scrolling/wheeling. As you said functionally it wouldn't make a difference, and I think it would require some unnecessary effort to facilitate it
| startAnimation() { | ||
| selectRefs(this.contentSpanBar, (spanBarDOM: HTMLDivElement) => { | ||
| spanBarDOM.style.transition = 'transform 0.3s'; | ||
| }); | ||
|
|
||
| if (this.animationTimeout) { | ||
| clearTimeout(this.animationTimeout); | ||
| } | ||
|
|
||
| // This timeout is set to trigger immediately after the animation ends, to disable the animation. | ||
| // The animation needs to be cleared, otherwise manual horizontal scrolling will be animated | ||
| this.animationTimeout = setTimeout(() => { | ||
| selectRefs(this.contentSpanBar, (spanBarDOM: HTMLDivElement) => { | ||
| spanBarDOM.style.transition = ''; | ||
| }); | ||
| this.animationTimeout = null; | ||
| }, 300); | ||
| } | ||
|
|
||
| disableAnimation() { | ||
| selectRefs(this.contentSpanBar, (spanBarDOM: HTMLDivElement) => { | ||
| spanBarDOM.style.transition = ''; | ||
| }); | ||
| } |
There was a problem hiding this comment.
I don't think you need any of this, just put a static transition: transform 0.3s; on the element style wherever it is defined, it's okay if it's css animating during wheel movements, it won't look that weird I think.
There was a problem hiding this comment.
@k-fish It actually makes it quite obnoxious to use unfortunately, I gave it a try when I first set this up lol
2 participants
This PR adds a new feature to the span tree, that will cause the view to adjust horizontally as the user scrolls down the tree. As a span goes out of the view, it will adjust by shifting all spans to the left so that the spans that are still in the view are given more visibility.
Similarly, if a span that was previously out of view comes back in the view (from the user scrolling up), the tree will readjust the spans to the right.
Demo
Addresses PERF-1429