Fix scrolling in variables view for web #5513
Conversation
There was a problem hiding this comment.
Vertical scroll is working for me on Mac in a Server Web dev build 👍
In Web and Desktop, I noticed there's some left/right scrolling as well when I scroll vertically, likely because I'm using a touchpad instead of a mouse wheel. It doesn't seem to be because of limited horizontal space -- perhaps a separate issue if it's not a quick fix, as this is an existing problem!
Also separate, a UX improvement thought: maybe we could "freeze" the expanded object/dataframe/etc title row until the user scrolls through all the nested contents. For example, in the video, we could freeze the df row, and then also the Petal.Length rows until we scroll through the nested elements.
variables_vertical_scroll_left_right.mp4
| @@ -513,6 +513,17 @@ export const VariablesInstance = (props: VariablesInstanceProps) => { | |||
| } | |||
| }; | |||
|
|
|||
| // workaround for web disabling scrolling on the window to prevent URL navigation | |||
| useEffect(() => { | |||
There was a problem hiding this comment.
Curious why we're using useEffect here instead of adding an onWheel to the div? I think the other places we've added this fix used onWheel.
There was a problem hiding this comment.
I don't know if it's better this way but it makes sense to be consistent everywhere. I just happened to be trying out different ways to figure out the correct elements to add the wheel listener since the List component doesn't expose its div container.
| if (!isWeb) { | ||
| return; | ||
| } | ||
|
|
||
| outerRef.current?.addEventListener('wheel', (e: WheelEvent) => { | ||
| innerRef.current.parentElement?.scrollBy(e.deltaX, e.deltaY); | ||
| }); |
There was a problem hiding this comment.
Is there a reasonable way to pull this onWheel logic into a util function that can be reused across all the places we are adding this handling? It could make it easier to keep track of the places we have this handling, as well as have a single place to update the implementation if needed.
There was a problem hiding this comment.
I think the easiest way to search for all instances currently is using the search term scrollBy(e.deltaX, e.deltaY)
There was a problem hiding this comment.
I think creating a custom React hook might be able to fulfill this. As long as it has access to the element that receives the scroll event and the element that needs to receive the scroll data (they can be different). This might be an easier way to deal with this bug over using the Scrollable so that it's more React compatible.
I noticed this too and it also exists on desktop. I think the styling or sizing on the content is 1px off. |
Address #5473
Adds a mouse wheel listener to the div containing list and forward it to the
Listcomponent. I tried to wrap everything in aScrollablebut it would take more time to get everything working the same way. TheListcomponent does some extra list virtualization for performance that I didn't want to risk touching that theScrollablewould have to replace.QA Notes
Creating enough variables to fill more than the Variables view is one way to test. Another is creating a dataframe and expanding some of the data in it.