-
Notifications
You must be signed in to change notification settings - Fork 108
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
Improve ShowkaseBrowser codebase and update navigation stack #390
base: master
Are you sure you want to change the base?
Conversation
shape = MaterialTheme.shapes.large | ||
// This is added to make sure that the navigation of the ShowkaseBrowser does not break | ||
// when one of the previews has a back press handler in the implementation of the component. | ||
val backPressedDispatcherOwner = rememberOnBackPressedDispatcherOwner() |
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.
Doing this at the top level (which was previously the case) was causing the navigation to not work at all. Instead, I moved this down to specifically happen for the Composables that are responsible for rendering the component previews as that's where we want to override/intercept any back handling that the previews might be doing themselves. Doing so fixed the issue but I'm not 100% on why doing this at the top level didn't work okay.
ShowkaseCategory.TYPOGRAPHY -> navController.navigate( | ||
ShowkaseCurrentScreen.TYPOGRAPHY_GROUPS | ||
) | ||
ShowkaseCategory.COMPONENTS -> onNavigateToComponentGroups() |
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.
It would be nice if we could check here whether the component group only has one component and directly navigate into it.
groupedComponentMap = groupedComponentMap, | ||
showkaseBrowserScreenMetadata = showkaseBrowserScreenMetadata, | ||
onUpdateShowkaseBrowserScreenMetadata = onUpdateShowkaseBrowserScreenMetadata, | ||
navigateTo = navigateTo, | ||
) | ||
} | ||
composable(ShowkaseCurrentScreen.COMPONENT_STYLES.name) { | ||
ShowkaseComponentStylesScreen( |
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.
Similarly, skipping the styles screen when there's only one style would be super nice. Directly to the detail screen.
@@ -61,7 +61,8 @@ dependencies { | |||
// Support Libraries | |||
implementation deps.support.appCompat | |||
implementation deps.support.ktx | |||
implementation deps.support.lifecycleExtensions | |||
implementation deps.support.lifecycleComposeViewModel | |||
implementation deps.support.lifecycleComposeRuntime |
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.
Space was lost here :)?
@@ -62,7 +62,8 @@ dependencies { | |||
// Support Libraries | |||
implementation deps.support.appCompat | |||
implementation deps.support.ktx | |||
implementation deps.support.lifecycleExtensions | |||
implementation deps.support.lifecycleComposeViewModel | |||
implementation deps.support.lifecycleComposeRuntime |
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.
Same here
@@ -94,7 +94,8 @@ dependencies { | |||
// Support Libraries | |||
implementation deps.support.appCompat | |||
implementation deps.support.ktx | |||
implementation deps.support.lifecycleExtensions | |||
implementation deps.support.lifecycleComposeViewModel | |||
implementation deps.support.lifecycleComposeRuntime |
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.
And here
The browser side of Showkase was written in ~March 2020 when documentation for Compose was non-existant and best practices weren't documented. A lot of the code did not meet the current standards and was causing breaking behavior when trying to update some libraries. I ended up going down a rabbit hole and decided to fix some of the obvious issues, although I can see some more obvious improvements that I'll do in the future (or would love someone to help and contribute if you are interested).
@airbnb/showkase-maintainers