feat: add core range, time and volume range - #23
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a proof of concept for extracting time range functionality into a core class that can be shared between React and HTML implementations. The change refactors the React TimeRange component to use a new CoreTimeRange class from a @vjs-10/core package, moving the component logic out of React-specific code.
Key changes:
- Creates a new
@vjs-10/corepackage with a framework-agnosticTimeRangeclass - Refactors the React TimeRange component to use the core class instead of local state management
- Uses underscore-prefixed state properties for bundler minification optimization
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/react/src/components/TimeRange.tsx | Refactored to use CoreTimeRange class, removing local pointer event handling and state management |
| packages/react/react/package.json | Added dependency on the new @vjs-10/core package |
| packages/core/core/tsdown.config.ts | Build configuration for the new core package |
| packages/core/core/tsconfig.json | TypeScript configuration for the core package |
| packages/core/core/tsconfig.build.json | Build-specific TypeScript configuration |
| packages/core/core/src/index.ts | Entry point exporting TimeRange class and types |
| packages/core/core/src/components/time-range.ts | Core TimeRange class implementation with event handling and state management |
| packages/core/core/package.json | Package configuration for the new core package |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const calculateSeekTimeFromPointerEvent = (e: PointerEvent, duration: number): number => { | ||
| if (!(e.currentTarget instanceof HTMLElement)) return 0; | ||
| const rect = e.currentTarget.getBoundingClientRect(); | ||
| const ratio = calculatePointerRatio(e.clientX, rect); | ||
| return calculateSeekTimeFromRatio(ratio, duration); | ||
| }; |
There was a problem hiding this comment.
The function returns 0 when currentTarget is not an HTMLElement, but this could mask errors. Consider throwing an error or using a more explicit default behavior.
luwes
changed the title
for minification
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
9097b6a to
022e2ce
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| useEffect(() => { | ||
| coreRef.current?.setState(state); | ||
| }, [...Object.values(state)]); |
There was a problem hiding this comment.
Creating a new array with Object.values(state) on every render is inefficient and may cause unnecessary re-runs. Consider using a more targeted dependency array with specific state properties or use useMemo to memoize the values.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This is a proof of concept for extracting a core class for both the React and HTML time range.
It's very rough without many things abstracted away.
Used a prefixed underscore for some state props so they can be minified by the bundler.