Conversation
c4cd1dd to
68aa47b
Compare
There was a problem hiding this comment.
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 <175728472+Copilot@users.noreply.github.com>
* 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 <175728472+Copilot@users.noreply.github.com> * 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 <175728472+Copilot@users.noreply.github.com> Co-authored-by: alexandersimoes <alexandersimoes@gmail.com>
…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
| 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} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
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