Skip to content
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

Jetcaster UI updates #1498

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Jetcaster UI updates #1498

wants to merge 9 commits into from

Conversation

simona-anomis
Copy link
Contributor

@simona-anomis simona-anomis commented Nov 6, 2024

Add to Jetcaster: shared elements, swipetodismiss, lazy list animations and removal of episode from queue

Screen_recording_20241106_141006.mp4

@simona-anomis simona-anomis self-assigned this Nov 6, 2024
@simona-anomis simona-anomis requested a review from a team as a code owner November 6, 2024 13:18
Copy link
Contributor

@mlykotom mlykotom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments

modifier = Modifier.fillMaxWidth()
)
val sharedTransitionScope = LocalSharedTransitionScope.current
?: throw IllegalStateException("No SharedElementScope found")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be more appropriate to have this throw in the composition local definition (this is the practice we use)

It would also mean you don't have to have it nullable

Comment on lines +362 to +366
val sharedTransitionScope = LocalSharedTransitionScope.current
?: throw IllegalStateException("No SharedElementScope found")
val animatedVisibilityScope = LocalAnimatedVisibilityScope.current
?: throw IllegalStateException("No SharedElementScope found")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as ☝🏼 comment

Comment on lines +133 to +141
when (dismissState.currentValue) {
SwipeToDismissBoxValue.EndToStart -> {
removeFromQueue(episode)
}

SwipeToDismissBoxValue.StartToEnd -> {
}

SwipeToDismissBoxValue.Settled -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're not caring about the other states, should we just use

if (dismissState.currentValue == SwipeToDismissBoxValue.EndToStart) {
            removeFromQueue(episode)
        }

) {
val playerEpisode = uiState.episodePlayerState
val currentEpisode = playerEpisode.currentEpisode ?: return

val sharedTransitionScope = LocalSharedTransitionScope.current
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this also be done in the non-regular PlayerContents for getting the same shared element transition for tabletop mode? Or maybe this could be done within the PlayerImage itself to avoid duplication?

mlykotom added a commit that referenced this pull request Nov 18, 2024
[Merge after #1498]

- This PR merges the home UI state into one data class to be able to
show loading state and content state at the same time.
- It also fixes the progress to wait for the loading to finish.
- Instead of circular progress, I switched to linear progress at the top
of the content, which can indicate the work while showing content

**After**


https://github.com/user-attachments/assets/ec3e9220-addc-4b96-9b6a-477294b6cf85




**Before**


https://github.com/user-attachments/assets/c795afc3-01f6-407b-b0b6-505da33652ec
@simona-anomis
Copy link
Contributor Author

As discussed previously, this PR needs to wait for the 1.8 stable and shared elements grid support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants