Add course player and verification screens #27
Conversation
islam-abdel-halem
commented
Jun 21, 2025
islam-abdel-halem
commented
- Implements the UI and ViewModel for the course player feature.
- Enhance verification screen.
- Includes new bookmark icons for the course player UI.
- Implements the UI and ViewModel for the course player feature. - Enhance verification screen. - Includes new bookmark icons for the course player UI.
There was a problem hiding this comment.
Pull Request Overview
This PR implements the UI and ViewModel scaffolding for a new course player feature, introduces bookmark icons, and refines the verification screen imports and icon usage.
- Added scalable vector drawables for bookmark icons.
- Created
CoursePlayerViewModel,CoursePlayerUiState, and composable UI inCoursePlayerScreen. - Updated
VerificationScreenimports and switched to auto-mirrored back icon.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/res/drawable/bookmarkborder.xml | Added vector for bookmark outline icon |
| app/src/main/res/drawable/bookmark.xml | Added vector for filled bookmark icon |
| app/src/main/java/org/mahd_e_learning_platform/presentation/screens/course_player/CoursePlayerViewModel.kt | Implemented UI state flow and toggle bookmark logic; stubbed TODOs |
| app/src/main/java/org/mahd_e_learning_platform/presentation/screens/course_player/CoursePlayerUiState.kt | Defined data models and default UI state |
| app/src/main/java/org/mahd_e_learning_platform/presentation/screens/course_player/CoursePlayerScreen.kt | Built composable layout for course player UI |
| app/src/main/java/org/mahd_e_learning_platform/presentation/screens/auth/verification/Verification.kt | Refined layout imports and switched to Icons.AutoMirrored.Filled.ArrowBack |
| android:width="800dp" | ||
| android:height="800dp" |
There was a problem hiding this comment.
The vector drawable width is set to 800dp, which is unusually large—standard Android vector icons typically use 24dp to match the viewportWidth. Consider changing to 24dp.
| android:width="800dp" | |
| android:height="800dp" | |
| android:width="24dp" | |
| android:height="24dp" |
| android:width="800dp" | ||
| android:height="800dp" |
There was a problem hiding this comment.
The vector drawable width is set to 800dp; use a more appropriate size such as 24dp to align with viewportHeight and common icon dimensions.
| android:width="800dp" | |
| android:height="800dp" | |
| android:width="24dp" | |
| android:height="24dp" |
| class CoursePlayerViewModel : ViewModel() { | ||
|
|
||
| private val _uiState = MutableStateFlow(CoursePlayerUiState()) | ||
| val uiState = _uiState |
There was a problem hiding this comment.
Expose the state as an immutable StateFlow rather than the mutable _uiState; e.g., val uiState: StateFlow<CoursePlayerUiState> = _uiState.asStateFlow() to prevent external modification.
| val uiState = _uiState | |
| val uiState = _uiState.asStateFlow() |
| } | ||
| Spacer(Modifier.height(4.dp)) | ||
| LinearProgressIndicator( | ||
| progress = { progress.progress }, |
There was a problem hiding this comment.
The LinearProgressIndicator expects a Float for the progress parameter, not a lambda. Pass progress.progress directly without curly braces.
| progress = { progress.progress }, | |
| progress = progress.progress, |
| IconButton(onClick = onPlayPause) { | ||
| Icon( | ||
| imageVector = Icons.Filled.PlayArrow, | ||
| contentDescription = "Play/Pause", |
There was a problem hiding this comment.
[nitpick] Hard-coded content descriptions can hinder localization; consider using stringResource(R.string.play_pause) for contentDescription.
2 participants
