-
Notifications
You must be signed in to change notification settings - Fork 2
feat: use native slider for playback (#88) #109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
c4cd1dd to
68aa47b
Compare
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.
Pull Request Overview
This PR replaces the custom DIY slider implementation with the native @react-native-community/slider component to improve maintainability, accessibility, and provide a more native feel for playback controls.
Key changes:
- Introduces new
PlaybackSlidercomponent wrapping@react-native-community/slider - Removes complex gesture handling code using
react-native-reanimatedandreact-native-gesture-handler - Simplifies the codebase by eliminating ~200 lines of custom slider logic
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/PlaybackSlider.tsx | New component wrapping the native slider with TrackPlayer integration |
| src/app/Schedule/ShowDetailsPage.tsx | Replaces custom progress bar/gesture code with PlaybackSlider component |
| src/app/Schedule/ArchivedShowView.tsx | Replaces custom progress bar/gesture code with PlaybackSlider component |
| package.json | Adds @react-native-community/slider dependency (v5.1.1) |
| package-lock.json | Lock file update for new dependency |
| ios/Podfile.lock | iOS native dependency update for slider component |
Comments suppressed due to low confidence (1)
src/app/Schedule/ShowDetailsPage.tsx:68
- The
useMemohere is unnecessary and potentially problematic. TheuseProgress()hook already returns a stable object reference that updates when progress changes. Wrapping it inuseMemowithprogressHookas a dependency doesn't provide any benefit since the hook result already handles its own memoization. Additionally,progressHookwill never be falsy/null in normal operation, so the fallback|| { position: 0, duration: 0 }is unnecessary.
Consider simplifying to:
const progress = useProgress();And remove the progressHook variable entirely.
const progressHook = useProgress();
const progress = useMemo(
() => progressHook || { position: 0, duration: 0 },
[progressHook],
);
Co-authored-by: Copilot <[email protected]>
* feat: stop playback, don't pause (#30) * fix: simplify styles * fix: use icons for home screen play/pause * Update src/app/Schedule/ArchivedShowView.tsx Co-authored-by: Copilot <[email protected]> * fix: pause archives, don't stop them * fix: live should stop, archive should pause * feat: allow jump/forward back on archive playback * fix: add missing utils file * chore: remove unused hook * fix: undo stop for preview * chore: remove unused import * fix: fix auto-paused state regression * fix: add jumpinterval options * adds missing event listeners to handle lock screen jump forward/backward controls * chore: lint * fix: update jump intervals from 15 to 30 --------- Co-authored-by: Copilot <[email protected]> Co-authored-by: alexandersimoes <[email protected]>
…ck-slider * origin/main: feat: add vertical scrollbars (#144) (#145) chore: add husky and lint-staged (#137) fix: add right padding to show artwork (#143) (#154) chore: fix old relative imports (#139) chore: remove unused currentShowRef chore: remove unneeded currentShow code refactor: simplify mainContent() call get current show from RecentlyPlayedService to fix highlighting refactor: split chained ternaries on RecentlyPlayed for readability
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.
Pull request overview
Copilot reviewed 4 out of 6 changed files in this pull request and generated 7 comments.
| export default function PlaybackSlider({ | ||
| styles, | ||
| onValueChange, | ||
| onSlidingComplete, | ||
| onSlidingStart, | ||
| }: { | ||
| styles?: StyleProp<ViewStyle>; | ||
| // Returns value in seconds | ||
| onValueChange?: (value: number) => void; | ||
| // Returns value in percentage (0 to 1) | ||
| onSlidingComplete?: (value: number) => void; | ||
| // Returns value in percentage (0 to 1) | ||
| onSlidingStart?: (value: number) => void; | ||
| }) { | ||
| const progress = useProgress(); | ||
|
|
||
| return ( | ||
| <Slider | ||
| style={styles} | ||
| minimumValue={0} | ||
| maximumValue={1} | ||
| onSlidingStart={onSlidingStart} | ||
| onSlidingComplete={onSlidingComplete} | ||
| thumbTintColor={ | ||
| Platform.OS === 'android' ? CORE_COLORS.WMBR_GREEN : undefined | ||
| } | ||
| minimumTrackTintColor={CORE_COLORS.WMBR_GREEN} | ||
| maximumTrackTintColor={COLORS.TEXT.PRIMARY} | ||
| onValueChange={value => | ||
| onValueChange?.(value * (progress?.duration || 0)) | ||
| } | ||
| tapToSeek={true} | ||
| value={progress.duration > 0 ? progress.position / progress.duration : 0} | ||
| /> | ||
| ); | ||
| } |
Copilot
AI
Dec 6, 2025
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.
The new PlaybackSlider component lacks test coverage. Since the repository includes comprehensive automated testing for components (as seen in __tests__/ directory with tests for PlayButton, BottomMenuBar, etc.), this new component should also have tests to verify its behavior, especially around the slider value calculations and callback interactions.
Replacing our DIY slider for OS-native sliders from React Native library. Should feel more native, be easier to maintain, and better for accessibility.
I can't actually drag this on the Android emulator, so -- I don't know if that's the emulator's fault or the component's fault, but either way I'm opening this as draft.
This will require an
npm iandpod installto work.Closes #88
Screen.Recording.2025-11-15.at.12.55.18.PM.mov