Feat(Timeline): Add timeOffset in options so that the scale can be aligned with data #3941
Conversation
WalkthroughThe changes introduce a new optional property, Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/plugins/timeline.ts (2)
27-28: Enhance documentation for timeOffset optionWhile the type definition is correct, the documentation comment could be more descriptive to help users understand:
- The unit of measurement (seconds)
- The effect on timeline display
- Example usage scenarios
Consider expanding the documentation:
- /** offset in seconds for the numeric labels */ + /** + * Offset in seconds for the timeline's numeric labels. + * Useful when audio doesn't start at 0 seconds, allowing timeline + * labels to align with actual audio timing. + * @example + * // If audio starts 2.5 seconds into the track + * timeOffset: 2.5 + */🧰 Tools
🪛 eslint
[error] 28-28: Delete
··(prettier/prettier)
148-151: Consider simplifying defaultTimeOffset implementationThe method currently:
- Takes an unused
pxPerSecparameter- Returns a hardcoded value
Consider one of these alternatives:
- private defaultTimeOffset(pxPerSec: number): number { - return 0; - } + // Option 1: Remove unused parameter + private defaultTimeOffset(): number { + return 0; + } + // Option 2: Define as a constant + private readonly DEFAULT_TIME_OFFSET = 0;🧰 Tools
🪛 eslint
[error] 148-148: Delete
··(prettier/prettier)
[error] 149-149: Delete
··(prettier/prettier)
[error] 149-149: 'pxPerSec' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 150-150: Replace
······return·0;with····return·0(prettier/prettier)
[error] 151-151: Delete
··(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/plugins/timeline.ts(4 hunks)
🧰 Additional context used
🪛 eslint
src/plugins/timeline.ts
[error] 28-28: Delete ··
(prettier/prettier)
[error] 148-148: Delete ··
(prettier/prettier)
[error] 149-149: Delete ··
(prettier/prettier)
[error] 149-149: 'pxPerSec' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 150-150: Replace ······return·0; with ····return·0
(prettier/prettier)
[error] 151-151: Delete ··
(prettier/prettier)
🔇 Additional comments (1)
src/plugins/timeline.ts (1)
192-192: Implementation looks good, verify with edge cases
The timeOffset integration is well implemented:
- Proper fallback to default value
- Correct application in notch positioning calculation
Let's verify the implementation handles edge cases correctly:
Also applies to: 259-259
katspaugh
changed the title
| private defaultTimeOffset(pxPerSec: number): number { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
This doesn't have to be a function, does it? It can be just a static value in defaultOptions.
I am using the timeline to display BPM (Beat's per minute) for a given sound file. But sound files don't always start exactly with beat 0 at 0 ms. options.timeOffset allows you to shift the labels up or down the timeline and line them up with the waveform.
I normally don't do git. In fact I just lost two days worth of work while fuckling around with git shit. but here you are.
I like your library though, it's easy to understand.
Summary by CodeRabbit
timeOffsetproperty for the timeline plugin, allowing users to customize the alignment of numeric labels.