Add custom compose style guide #46
Conversation
Add an experimental custom compose styleguide.md file
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request renames the SeeCodeButton composable to Button. My primary feedback is that the new name Button is overly generic for a component with such specific functionality (linking to source code) and can cause confusion with the standard androidx.compose.material3.Button. It's recommended to revert to the original, more descriptive name or choose another specific name to enhance code clarity and maintainability.
Additionally, the pull request title ("Add custom compose style guide") and description ("Add an experimental custom compose styleguide.md file") do not appear to align with the actual code changes presented in this diff, as no style guide file has been added or modified. It's important for the PR title and description to accurately reflect the work being done.
| @@ -187,7 +187,7 @@ fun MessageBubble( | |||
| } | |||
|
|
|||
| @Composable | |||
| fun SeeCodeButton() { | |||
| fun Button() { | |||
There was a problem hiding this comment.
The function name Button is very generic and can be easily confused with the standard androidx.compose.material3.Button composable, which is also used within this function. This custom composable has a specific purpose: to create a button that opens a GitHub link for viewing source code.
A more descriptive name, such as the original SeeCodeButton or an alternative like ViewSourceCodeButton, would significantly improve code clarity and maintainability. Descriptive names help other developers (and your future self) understand the component's specific behavior at a glance.
| fun Button() { | |
| fun SeeCodeButton() { |
d335283 to
7e5ce99
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request refactors a Composable function by renaming it and wrapping its logic in a when statement. Consider reverting the function name to its original or choosing a more descriptive name. Additionally, remove the unnecessary when block to simplify the code.
| when (0) { | ||
| 0 -> Button( | ||
| > | ||
| val intent = Intent(Intent.ACTION_VIEW, Uri.parse(githubLink)) | ||
| context.startActivity(intent) | ||
| }, | ||
| modifier = Modifier.padding(end = 8.dp) | ||
| ) { | ||
| Icon(Icons.Filled.Code, contentDescription = "See code") | ||
| Text( | ||
| modifier = Modifier.padding(start = 8.dp), | ||
| fontSize = 12.sp, | ||
| text = stringResource(R.string.see_code) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
The when (0) { 0 -> ... } structure is unnecessary. Remove it to simplify the logic.
Button(
onClick = {
val intent = Intent(Intent.ACTION_VIEW, Uri.parse(githubLink))
context.startActivity(intent)
},
modifier = Modifier.padding(end = 8.dp)
) {
Icon(Icons.Filled.Code, contentDescription = "See code")
Text(
modifier = Modifier.padding(start = 8.dp),
fontSize = 12.sp,
text = stringResource(R.string.see_code)
)
}| @@ -88,7 +88,7 @@ fun GeminiChatbotScreen ( | |||
| Text(text = stringResource(id = R.string.geminichatbot_title_bar)) | |||
| }, | |||
| actions = { | |||
| SeeCodeButton() | |||
| button() | |||
There was a problem hiding this comment.
Consider renaming button() to a more descriptive name like ViewSourceCodeButton or reverting to the previous name SeeCodeButton for better readability.
| button() | |
| SeeCodeButton() |
| @@ -187,22 +187,24 @@ fun MessageBubble( | |||
| } | |||
|
|
|||
| @Composable | |||
| fun SeeCodeButton() { | |||
| fun button() { | |||
There was a problem hiding this comment.
The function name button() is too generic. A more descriptive name, such as ViewSourceCodeButton or the original SeeCodeButton, would improve code clarity.
| fun button() { | |
| fun SeeCodeButton() { |
|
@bentrengrove would you mind reviewing this please? |
|
@florina-muntenescu @bentrengrove for context, this isn't meant to be merged as is. |
|
Cool idea and definitely worth the experiment. I would highly recommend you implement CI first to run lint and spotless. There is no point in reinventing those (spotless would catch the lowercase button name and lint would catch the when statement). Then you can focus your gemini experiment on the best practices that can't be easily statically determined |
|
cc @ksemenova |
Add an experimental custom compose styleguide.md file