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

update screen transition #428

Merged
merged 3 commits into from
Jun 3, 2024

Conversation

shpasha
Copy link
Contributor

@shpasha shpasha commented May 22, 2024

A slightly different approach to screen transitions. Now the active screen (the one we push) determines the animation. We already have this approach in our approach and it is very convenient.

I also added lastEvent to the enter/exit to understand the slide direction

sample.webm

@Composable
public fun ScreenTransition(
navigator: Navigator,
screenTransition: ScreenTransition,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

practice has shown that it is convenient to get a default transition as ScreenTransition argument here


import cafe.adriel.voyager.transitions.ScreenTransition

data class NoCustomAnimationSampleScreen(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we navigate to this screen, or pop from it, default animation will be used

override val index: Int,
) : BaseSampleScreen()

data class FadeAnimationSampleScreen(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we navigate to this screen, or pop from it, fade animation will be used

@shpasha shpasha force-pushed the improve-screen-transition branch from b9eb285 to c4d22da Compare May 22, 2024 07:42
@shpasha shpasha force-pushed the improve-screen-transition branch from c4d22da to 2a68388 Compare May 22, 2024 07:44
Copy link
Collaborator

@DevSrSouza DevSrSouza left a comment

Choose a reason for hiding this comment

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

Nice work! Looking great.

Comment on lines 25 to 32
public fun enter(isPop: Boolean): EnterTransition? = null

/**
* Defines the exit transition for the Screen.
*
* Returns null when it should not define a transition for this screen.
*/
public fun exit(): ExitTransition? = null
public fun exit(isPop: Boolean): ExitTransition? = null
Copy link
Collaborator

@DevSrSouza DevSrSouza May 22, 2024

Choose a reason for hiding this comment

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

I still not 100% sure if isPop is enough, there are cases where other events maters for transition besides pop? if so, we could pass the navigation last event directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@DevSrSouza
Copy link
Collaborator

Can you also update the docs in the repo? We have moved to the repo

@shpasha shpasha force-pushed the improve-screen-transition branch 4 times, most recently from 4b6530a to 683bd46 Compare May 24, 2024 11:28
@shpasha shpasha force-pushed the improve-screen-transition branch from 683bd46 to c195baf Compare May 24, 2024 12:03
@shpasha
Copy link
Contributor Author

shpasha commented May 29, 2024

@DevSrSouza If there are no more problems, maybe we can merge the request?

@DevSrSouza
Copy link
Collaborator

DevSrSouza commented Jun 3, 2024

@shpasha Can you update this docs @shpasha https://github.com/adrielcafe/voyager/blob/main/docs/transitions-api.md ?

Adding the new ScreenTransition function and some examples using delegate implementation, etc.

@shpasha
Copy link
Contributor Author

shpasha commented Jun 3, 2024

@DevSrSouza docs updated

@DevSrSouza
Copy link
Collaborator

DevSrSouza commented Jun 3, 2024

Awesome work, thanks for the contribution

@DevSrSouza DevSrSouza merged commit 486497c into adrielcafe:main Jun 3, 2024
1 of 2 checks passed
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.

None yet

2 participants