[WIP] Migrate Design System components in Jetsnack to use Styles#1669
[WIP] Migrate Design System components in Jetsnack to use Styles#1669
Conversation
riggaroo
requested review from
AjeshRPai,
MagicalMeghan,
dturner,
raystatic,
simona-anomis and
srikrishnasakunia
as code owners
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request begins the significant architectural shift in the Jetsnack application by migrating its custom Design System components to leverage the new Compose Styles API. This change aims to streamline UI development, improve maintainability, and provide a more robust and flexible styling mechanism for various UI elements, laying the groundwork for a more scalable design system implementation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors various UI components (Button, Card, Divider, FilterChip, GradientTintedIconButton, QuantitySelector, Snack-related components, and Surface) in the Jetsnack app to adopt a new experimental Compose Foundation Style API. This involves introducing a Style parameter to these components and migrating their visual properties (like shape, background, content color, padding, and text styles) into Style objects. A new JetsnackText composable is introduced to facilitate text styling within this system. Additionally, a temporary CompositionLocals.kt file is added to access internal Material 3 theming locals via reflection, and AppStyles is introduced to centralize default styles. Review comments highlight critical issues where StyleScope functions like contentPaddingHorizontal, contentPaddingTop, and fillWidth are incorrectly used, leading to compilation errors. There's also a high-severity concern regarding the use of reflection for internal Material 3 APIs, which is brittle, and a suggestion to use more specific exception handling. Finally, an issue with ButtonDefaults.ContentPadding.calculateTopPadding() being incorrectly passed to contentPadding is noted.
| style = { | ||
| jetsnackTextStyle(LocalTypography.currentValue.headlineMedium) | ||
| contentColor(LocalJetsnackColors.currentValue.textHelp) | ||
| contentPaddingHorizontal(24.dp) | ||
| } |
There was a problem hiding this comment.
The function contentPaddingHorizontal is not a valid function on StyleScope. This will cause a compilation error. You should use the contentPadding function with named arguments.
| style = { | |
| jetsnackTextStyle(LocalTypography.currentValue.headlineMedium) | |
| contentColor(LocalJetsnackColors.currentValue.textHelp) | |
| contentPaddingHorizontal(24.dp) | |
| } | |
| style = { | |
| jetsnackTextStyle(LocalTypography.currentValue.headlineMedium) | |
| contentColor(LocalJetsnackColors.currentValue.textHelp) | |
| contentPadding(horizontal = 24.dp) | |
| } |
| style = { | ||
| jetsnackTextStyle(LocalTypography.currentValue.bodyLarge) | ||
| contentColor(LocalJetsnackColors.currentValue.textHelp) | ||
| contentPaddingHorizontal(24.dp) | ||
| }, |
There was a problem hiding this comment.
The function contentPaddingHorizontal is not a valid function on StyleScope. This will cause a compilation error. You should use the contentPadding function with named arguments.
| style = { | |
| jetsnackTextStyle(LocalTypography.currentValue.bodyLarge) | |
| contentColor(LocalJetsnackColors.currentValue.textHelp) | |
| contentPaddingHorizontal(24.dp) | |
| }, | |
| style = { | |
| jetsnackTextStyle(LocalTypography.currentValue.bodyLarge) | |
| contentColor(LocalJetsnackColors.currentValue.textHelp) | |
| contentPadding(horizontal = 24.dp) | |
| }, |
| style = { | ||
| jetsnackTextStyle(LocalTypography.currentValue.labelLarge) | ||
| contentColor(LocalJetsnackColors.currentValue.textLink) | ||
| textAlign(TextAlign.Center) | ||
| contentPaddingTop(15.dp) | ||
| fillWidth() | ||
| }, |
There was a problem hiding this comment.
The functions contentPaddingTop and fillWidth are not valid on StyleScope. This will cause a compilation error. You should use contentPadding(top = 15.dp) and fillMaxWidth() instead.
| style = { | |
| jetsnackTextStyle(LocalTypography.currentValue.labelLarge) | |
| contentColor(LocalJetsnackColors.currentValue.textLink) | |
| textAlign(TextAlign.Center) | |
| contentPaddingTop(15.dp) | |
| fillWidth() | |
| }, | |
| style = { | |
| jetsnackTextStyle(LocalTypography.currentValue.labelLarge) | |
| contentColor(LocalJetsnackColors.currentValue.textLink) | |
| textAlign(TextAlign.Center) | |
| contentPadding(top = 15.dp) | |
| fillMaxWidth() | |
| }, |
| private fun <T> getInternalLocal(className: String, methodName: String, readableName: String): ProvidableCompositionLocal<T> { | ||
| try { | ||
| val clazz = Class.forName(className) | ||
| val method: Method = clazz.getDeclaredMethod(methodName) | ||
| method.isAccessible = true | ||
| @Suppress("UNCHECKED_CAST") | ||
| return method.invoke(null) as ProvidableCompositionLocal<T> | ||
| } catch (e: Exception) { | ||
| throw RuntimeException("Failed to access internal $readableName via reflection", e) | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Using reflection to access internal library APIs is highly discouraged as it's brittle and can break without warning on any library update, even minor ones. This makes the app fragile.
Also, catching a generic Exception is too broad. It would be safer to catch specific reflection-related exceptions like ClassNotFoundException, NoSuchMethodException, etc., to have a better understanding of what went wrong.
| shape( RoundedCornerShape(percent = 50)) | ||
| background(Brush.linearGradient(LocalJetsnackColors.currentValue.interactivePrimary)) | ||
| contentColor(LocalJetsnackColors.currentValue.textInteractive) | ||
| contentPadding( ButtonDefaults.ContentPadding.calculateTopPadding()) // todo file issue to support padding values |
There was a problem hiding this comment.
The contentPadding function is being called with a single Dp value returned by calculateTopPadding(). This will set the padding for all four sides to be the value of the top padding, which is likely not the intended behavior. To apply the default button content padding, you should pass the PaddingValues object directly.
| contentPadding( ButtonDefaults.ContentPadding.calculateTopPadding()) // todo file issue to support padding values | |
| contentPadding(ButtonDefaults.ContentPadding) |
1 participant
Known issues: https://issuetracker.google.com/492528450