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

Issue-221 Skip categories screen in case of having only components #222

Merged
merged 17 commits into from
Dec 20, 2022

Conversation

themanol
Copy link
Contributor

showkase_components_only

@marcorighini
Copy link
Contributor

What do you think about passing this option as a flag in the showkase activity intent? @vinaygaba @themanol
E.g. pass a configuration data class to the showkase activity intent that contains some configurations, i.e. atm skipSingleScreens (naming to be improved)

@marcorighini
Copy link
Contributor

@vinaygaba @themanol Do you think the suggestion above makes sense?

@vinaygaba
Copy link
Collaborator

@themanol I've somehow missed seeing this PR. Truly apologize for that. I'm ready to merge it once the builds pass.

@themanol
Copy link
Contributor Author

themanol commented Oct 7, 2022

@themanol I've somehow missed seeing this PR. Truly apologize for that. I'm ready to merge it once the builds pass.

The build pass, we need a review here, Thanks! @vinaygaba

@marcorighini
Copy link
Contributor

Yes, looks like can be merged now. Would be great to have a new release with this.

@vinaygaba
Copy link
Collaborator

vinaygaba commented Dec 17, 2022

What do you think about passing this option as a flag in the showkase activity intent? @vinaygaba @themanol
E.g. pass a configuration data class to the showkase activity intent that contains some configurations, i.e. atm skipSingleScreens (naming to be improved)

@marcorighini I think it probably makes sense to have this as the default behavior instead of making it configurable. If a lot of people think making it configurable is valuable, I'm open to reconsidering it.

NavHost(
navController = navController,
startDestination = ShowkaseCurrentScreen.SHOWKASE_CATEGORIES.name
) {
startDestination = if (isOnlyComponents) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should make this generic i.e directly jump to the respective groups screen if only one type of elements are present.

For example, if there are only colors, it should directly go to the colors groups screen. Same with typography or any other element that we might add support for in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, it makes sense, I can change this to be the default behaviour

Copy link
Collaborator

@vinaygaba vinaygaba left a comment

Choose a reason for hiding this comment

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

Sorry about dropping on the ball on this PR and not reviewing it earlier.

Copy link
Collaborator

@vinaygaba vinaygaba left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this!

@vinaygaba vinaygaba merged commit 0870aea into airbnb:master Dec 20, 2022
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

3 participants