SparkButton icon indicating copy to clipboard operation
SparkButton copied to clipboard

Library Feedback

Open evant opened this issue 2 years ago • 1 comments

Saw your announcement post and wanted to give some feedback. Happy to break this out into multiple issues if that's more helpful.

  1. Would recommend taking a @Composable content instead of an icon. This allows more flexibility on what you can display and can simplify the caller implementation. For example, using material3's Icon (also solves the problem of not being able to provide a content description):
SparkButton(onClick = {...}) {
   Icon(Icons.Default.Favorite, contentDescription = "Favorite")
}
  1. The active param seems a little strange to me, isn't the point of the library to show the animation? It's very easy in compose to swap things around if you only need it conditionally:
if (animate) {
  SparkButton(onClick = {...}) {
     FavoiteIcon()
  }
} else {
   IconButton(onClick = {...}) {
      FavoiteIcon()
   }
}

if there is a good reason for it, could it default to true at least?

  1. You want to pass in a Role of Button for accessibility, you can do this on the clickable modifier.
modifier.clickable(role = Role.Button)
  1. Since this is a custom button implementation you may want to implement a disabled state that doesn't take a click interaction. You can actually pass this to the clickable modifier as well
fun SparkButton(..., enabled: Boolean = true) {
...
modifier.clickable(role = Role.Button, enabled = enabled)

evant avatar Sep 11 '23 21:09 evant

Oh that is super helpful, thank you very much 😍

connyduck avatar Sep 16 '23 16:09 connyduck

I adressed most things, the api is now similar to androidx.compose.material3.IconToggleButton which means it takes a @Composable content, can be enabled and disabled, and active has been inverted and renamed to checked.

connyduck avatar Mar 13 '24 10:03 connyduck