feat(widgets): minimal TimelineWidget (PoC) #9587
Conversation
There was a problem hiding this comment.
Thanks for putting this together. I left a few comments, but overall I think this is a solid POC. Let’s export it as an experimental for now - I’m not yet convinced this will be the final API.
I’d like to see additional explorations that better support handling date-time domains, and I’m unsure about exposing step as a prop.
It looks a bit like this widget is blending two separate concerns: a generic value slider and a timeline-style scrubber with time-domain semantics (e.g. time intervals, playhead). I’d recommend splitting these into separate widgets and keeping this one focused on time semantics.
| this.props.onTimeChange(next); | ||
| this.renderUI(); | ||
| if (this.playing) { | ||
| this.timerId = window.setTimeout(this.tick, this.props.playInterval); |
There was a problem hiding this comment.
setTimeout works well for longer intervals like 1000ms and is easy to customize, but it’s less consistent for smooth playback. A requestAnimationFrame-based approach, like luma’s Timeline, provides smoother results.
Using setTimeout for a POC is fine.
| if (this.element) { | ||
| // update className | ||
| this.element.className = ['deck-widget', 'deck-widget-timeline', this.props.className] | ||
| .filter(Boolean) | ||
| .join(' '); | ||
| // update style | ||
| if (props.style) { | ||
| Object.assign(this.element.style, this.props.style); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use the same CSS update method as the other widgets. This method makes unnecessary mutations and doesn't support removing styles as you'd expect.
https://github.com/visgl/deck.gl/blob/master/modules/widgets/src/fullscreen-widget.tsx#L111-L121
| new FullscreenWidget({style: widgetTheme}), | ||
| new TimelineWidget({ | ||
| style: widgetTheme, | ||
| domain: [0, 24], |
There was a problem hiding this comment.
A gotcha to include in our docs is the same gotcha that we document in TripsLayer - it's typical for time domains to overflow integers, so users should be mindful and apply an offset
| /** | ||
| * Current slider value. | ||
| */ | ||
| value?: number; |
There was a problem hiding this comment.
| value?: number; | |
| currentTime?: number; |
Consider using time oriented names
| /** | ||
| * Slider domain [min, max]. | ||
| */ | ||
| domain?: [number, number]; |
There was a problem hiding this comment.
| domain?: [number, number]; | |
| timeDomain?: [number, number]; |
Consider using time oriented names and also supporting [Date, Date]?
| /** | ||
| * Slider step. | ||
| */ | ||
| step?: number; |
There was a problem hiding this comment.
Why do we need this? It sounds redundant to interval.
|
Linked the feedback into the tracker, will land for now |
Closes #
Background
Having at least some kind of minimal timeline widget seems essential to a code widget offering.
This is an extremely minimal PoC, just to get some basic working code in place.
TODO
Change List