airbnb / Showkase

πŸ”¦ Showkase is an annotation-processor based Android library that helps you organize, discover, search and visualize Jetpack Compose UI elements
https://medium.com/airbnb-engineering/introducing-showkase-a-library-to-organize-discover-and-visualize-your-jetpack-compose-elements-d5c34ef01095
Apache License 2.0
2.11k stars 107 forks source link

Make stacked preview annotations generate showkaseMetadata components for each annotation #259

Closed oas004 closed 2 years ago

oas004 commented 2 years ago

This is the next step in supporting Multipreview annotations. Referencing #255 as the first step.

Here I have added functionality to make a ShowkaseMetadata object for each Preview annotation. Eg.

@Preview(
    name = "small font",
    group = "font scales",
    fontScale = 0.5f
)
@Preview(
    name = "large font",
    group = "font scales",
    fontScale = 1.5f
)
@Composable
fun ComposablePreviewFont() {

}

Before, this would only make one component in Showkase. Now it should make one for each preview and place them in their respective group. Here they would both be placed in the group font scales

Furthermore, I would advice to review this by commit as I had to update the tests because of some arrangement due to a check in the processor. I placed the test update in 8371ec2efd81e6317c85029e377dca3ae8cd0aee.

I'm leaving this as a draft PR because I'm having some issues with getting this to work with KAPT. With KSP, the method roundEnvironment.getElementsAnnotatedWith(PREVIEW_CLASS_NAME) returns all the elements for multi stacked preview. However, with KAPT, it does not seem to work the same way. Is there some other function I should use here? Any suggestions are more than welcome πŸ˜„ Added a slack post for anyone interested πŸ˜„

oas004 commented 2 years ago

This is looking in good shape. You can do the ShowkaseComposable support later on as it's not super pressing. I think you are at a point where you can move to the custom annotation stuff we discussed previously. Great job in making such a critical feature happen πŸ‘πŸ»

Thank you very much @vinaygaba! And thank you so much valuable review πŸ™ I tried to add support for stacked @ShowkaseComposable in b109238. I also wrote some docs about these not working with KAPT because of https://youtrack.jetbrains.com/issue/KT-49682 . Furthermore, added tests for only KSP for stacked @Preview and @ShowkaseComposable cases πŸ˜ƒ

vinaygaba commented 2 years ago

@oas004 Amazing! One final thing you can do is write a test for the ShowkaseBrowser that verifies the following:

Add the test cases here - https://github.com/airbnb/Showkase/blob/master/showkase-browser-testing/src/androidTest/java/com/airbnb/android/showkase_browser_testing/ShowkaseBrowserTest.kt

oas004 commented 2 years ago

@oas004 Amazing! One final thing you can do is write a test for the ShowkaseBrowser that verifies the following:

  • Stacked previews show up in the browser
  • They are being treated as separate previews

Add the test cases here - https://github.com/airbnb/Showkase/blob/master/showkase-browser-testing/src/androidTest/java/com/airbnb/android/showkase_browser_testing/ShowkaseBrowserTest.kt

That is a very good idea! πŸ™Œ Is there a way for us to bypass it in KAPT on CI? I think if I added those BrowserTests, then it will fail on KAPT every time πŸ€”

vinaygaba commented 2 years ago

@oas004 Oh no, you are right. We currently run these tests for both ksp and kapt and pass the PuseKsp flag from CLI. Is there a way to access this value in the test case somehow? If there is, we can always pass the test when it's kapt and run the test as usual when its ksp. This is how we run these tests - https://github.com/airbnb/Showkase/blob/master/.github/workflows/android.yml#L59

vinaygaba commented 2 years ago

@oas004 Can you try this - in the android.yml file, also add -DuseKsp=true and -DuseKsp=false and then try accessing them in the new test cases using System.getProperty("useKsp")

oas004 commented 2 years ago

@oas004 Can you try this - in the android.yml file, also add -DuseKsp=true and -DuseKsp=false and then try accessing them in the new test cases using System.getProperty("useKsp")

I think that can work! Will test this out first thing in the morning😊

oas004 commented 2 years ago

@vinaygaba Can we re-run the UI Tests? It didn't provide me a log to check if it worked or not πŸ˜…

oas004 commented 2 years ago

@vinaygaba Can we re-run the UI Tests? It didn't provide me a log to check if it worked or not πŸ˜…

it seems like it was failing on the screenshot tests πŸ€” I think I might need to take that into account as well somehow πŸ€”

oas004 commented 2 years ago

It seems like the KAPT stage is working on all but one emulator and then fails, so I can't see the others that checks green to verify if the System.getProperty trick works. I can't get it to work locally either though, so maybe I could try to save the variable in the gradle file as well? Or would that just be dumb? πŸ€”

oas004 commented 2 years ago

Very exciting! Thank you for your patience and getting this to the finish line πŸ‘

This is supercool! I will start on the actual mutlipreview annotation as soon as possible! πŸ˜„ 🀩 Thank you so much for your valuable input! πŸ™ Feel like I am learning a lot from this! πŸ˜ƒ 😍