fix(codeblock): fix height of codeblock to use DOM recycling scrolling #3711
Conversation
Signed-off-by: John Cowen <john.cowen@konghq.com>
✅ Deploy Preview for kuma-gui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @click="refresh" | ||
| <div | ||
| ref="$el" | ||
| > |
There was a problem hiding this comment.
Bit annoyed about this, the return of meaningless divs.
It seems in order to use a ref to get a reference deep in your component tree (in this case *View.vue > XCodeBlock > XLayout), you have to do a load of defineExpose plumbing, which is just going to be brittle and hard to maintain, and at the very least annoying to have to use so much code to get to a deep DOM element.
Instead I just figured I could just wrap the thing in an extra div and get the position of the extra div instead (we need the .top of the XCodeBlock to calculate the height it should be)
| ref="$el" | ||
| > | ||
| <XCodeBlock | ||
| :max-height="`${(resize?.target?.innerHeight ?? 0) - ($el?.getBoundingClientRect().top + 146)}`" |
There was a problem hiding this comment.
Ideally we don't want to hardcode this number, but at the moment it's not obvious how to calculate this exactly, I suspect KCodeBlock's maxHeight doesn't take into account its toolbar.
All in all in order to move forwards, I'd be happy with this magical number for the moment at least. Take into account we'll need to do this in three pages, XDS Config, Stats and Clusters.
There was a problem hiding this comment.
Would it make sense to define a min-height also?
There was a problem hiding this comment.
🤔 I think from a UX perspective that would potentially result in 2 scrollbars (one on the codeblock one on the window), which I reckon we should avoid
Technically, notice we the max-height is a "feature" of the underlying KCodeBlock, so if we'd want to add a min-height we'd have to add some custom CSS, which in some cases I'd be fine with but considering the above UX point also, I'd definitely be on the side of not adding extra bits.
There was a problem hiding this comment.
Hmmm, in my case I already see two scrollbars (not sure what is causing this). But I don't think that is bad in general. In case there is more stuff added to that view, we need to have another scrollbar. So regardless of the scrollbars I think it would be best to give it a range in which in can grow to improve performance and usability 🤔
There was a problem hiding this comment.
I increased the "magic" number to 200, hopefully you only get one scrollbar now. This is why I'd rather it be calculated, but still I'd be happy with a magic number at least short term. The 2 scrollbars (if they should ever happen) are less bad than a slow search.
There was a problem hiding this comment.
I increased the "magic" number to 200, hopefully you only get one scrollbar now. This is why I'd rather it be calculated, but still I'd be happy with a magic number at least short term. The 2 scrollbars (if they should ever happen) are less bad than a slow search.
The second scrollbar is gone 🙂
Yeah that "magic" number is not ideal, but should be fine for now. Unfortunately it's not that easy to just let it grow to fill the rest of the screen, at least from my quick fiddling around 🤔
There was a problem hiding this comment.
Yeah that "magic" number is not ideal, but should be fine for now. Unfortunately it's not that easy to just let it grow to fill the rest of the screen, at least from my quick fiddling around
Yeah for sure, I couldn't find a calculation that was always correct, I'm happy to go with this for now also. Let me roll it out to the other pages, 🤔 we could make an issue for this maybe, brb
There was a problem hiding this comment.
This file wraps over GlobalEvents (a 3rd party package I added):
- In case we ever want to move, or roll our own.
- Maintains our "
xis for GUI related compoennts" - Means we can tailor it to our needs, currently we only support window resize events. If we need more we can add them incrementally as and when we need them here.
|
Taking this out of draft so its clear we are all happy with the approach, there is still work required before we merge, but for the moment we just need to make sure we are all happy with the general approach |
Signed-off-by: John Cowen <john.cowen@konghq.com>
Also see #3152, specifically #3152 (comment)
Looks like the line numbers we render here can cause a massive slowdown in re-renders, specifically when typing to search large configs.
The upstream KCodeBlock that we use for this does have a virtual scroller for the line numbers built in, but because of the nature of some virtual scrollers, and definitely in this case, you need to fix the height of the container of the thing that contains the virtual scroller. This means that the KCodeBlock now controls the scrolling, not the browser window.
Note: I only implemented this in the "full page" XDS Config pages for now, so if you are viewing the PR preview make sure you are looking at it on this page.