-
Notifications
You must be signed in to change notification settings - Fork 439
Conversation
@@ -183,8 +190,10 @@ export default function PlaybackControls(props: { | |||
<RepeatAdapter play={play} seek={seek} repeatEnabled={repeat} /> | |||
<KeyListener global keyDownHandlers={keyDownHandlers} /> | |||
<div className={classes.root}> | |||
<Scrubber onSeek={seek} /> | |||
<Stack direction="row" alignItems="center" flex={1} gap={1} overflowX="auto"> | |||
<div className={classes.scrubberWrapper}> |
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.
What's the purpose of adding a wrapper here?
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.
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.
It allows the scrubber to remain full width while the other elements scroll
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.
I’m confused — I thought the point of the overflow:hidden was to disable scrolling. Why is there still a scrollbar?
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.
We've disabled scrolling on the body but we still allow scrolling on the playback bar controls
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.
My gut feeling would be that we don't want to allow scrolling on the playback bar since it's part of the app "chrome" that is supposed to remain fixed. I think it could make more sense to start wrapping or hiding the controls if they don't fit. (However I also don't think we have any particularly strong goals right now about making this work nicely on tiny screens?)
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.
I'm not sure how to make this work in chrome (it seems to limit the width when I use the dev tools in responsive mode) but in Safari this is a pretty weird experience, especially since the buttons scroll but the scrubber does not:
Screen.Recording.2024-01-17.at.4.28.07.PM.mov
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.
since the buttons scroll but the scrubber does not
The scrubber is position sticky and full width and we are not truncating it. The buttons scrolling has been the behavior since we redsigned the playback bar ages ago. I guess the hope is that we at least make the content accessible even if it's not an ideal UX
LR: visually does the good stuff |
User-Facing Changes
None
Description
Resolves bug where window frame scrolls when tooltip is present inside PlotPanel