-
Notifications
You must be signed in to change notification settings - Fork 3
Image support #78
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
base: main
Are you sure you want to change the base?
Image support #78
Conversation
…both XML and Compose.
…work backend to OkHttp.
…from OkHttp to Ktor 3.
gabrielbmoro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, in general it looks awesome!! Ty @jacksonfdam for all the effort to make CraftD even better!!
| } | ||
| } | ||
|
|
||
| private fun String.toContentScale(): ContentScale = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Niceeee!! 👏🏼
| } | ||
| } | ||
|
|
||
| private fun String.toContentScale(): ContentScale = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#suggestion
We could add some unit tests for this toContentScale. I think it would be awesome! For sure, for that we need to make it internal
| import androidx.compose.runtime.Composable | ||
| import androidx.compose.ui.Modifier | ||
| import androidx.compose.ui.layout.ContentScale | ||
| import coil3.compose.AsyncImage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#discussion
Hey folks! I’ve been thinking about something: imagine someone adds Craftd to a project that’s already using a very recent version of Coil—or even another image-loading library.
Wouldn’t it make sense for us to introduce an abstraction layer so developers can plug in whichever image loader they prefer? This way, Craftd wouldn’t need updates every time a new Coil 3 release comes out, and teams would have more flexibility overall.
What do you think? Does this direction make sense to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of agree, but actually, just Coil is 100% compatible with Multiplatform
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem that this lib has support to android 100% too.
I agree with moro, i was thinking
I was thinking, I've had to do similar things using those server-driven UIs I made.
If you create a constructor in the Builder with a function that propagates to the component, like:
class CraftDImageBuilder(override val key: String = CraftDComponentKey.IMAGE_COMPONENT.key, loadImage : (String) -> Loader( algo que seja comum entre as libs teria que ver cada para pensar o objetvo certo aqui)) :
CraftDBuilder {
@Composable
override fun craft(model: SimpleProperties, listener: CraftDViewListener) {
val imageProperties = model.value.convertToElement<ImageProperties>()
imageProperties?.let {
CraftDImage(it, loadImage) { imageProperties.actionProperties?.let { listener.invoke(it) } }
}
}
}that accepts any kind of "result" from a coil or Picasso or something like that that the person implements, then when registering the components in the CraftedManager, the person could pass an implementation block using
and in the component there would be something inside the ImageView block, a loadImage.invoke(url) in Imageview
and when you register the component you can do this
val craftdBuilderManager = remember {
CraftDBuilderManager().add(
MySampleButtonComposeBuilder(),
CraftdIMageBuilder(){ url ->
//call your frameworker and return
}
)
}
LaunchedEffect(Unit) {
vm.loadProperties()
}
CraftDynamic(
properties = properties,
craftDBuilderManager = craftdBuilderManager
) {
println(
">>>> category ${it.analytics?.category} -" + " action ${it.analytics?.action} -" + " label ${it.analytics?.label} -" + " deeplink ${it.deeplink}"
)
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inside from craftdManager will be override de current CraftdImageBuilder default to new instancia
fun add(vararg arrayCraftDBuilder: CraftDBuilder) : CraftDBuilderManager{
arrayCraftDBuilder.forEach {
mapBuilder[it.key] = it
}
return this
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another thing
This PR you miss to register in CraftdManager your builder
CraftDBuilderManager
@Stable
@Immutable
class CraftDBuilderManager {
private val mapBuilder = hashMapOf(
CraftDComponentKey.BUTTON_COMPONENT.key to CraftDButtonBuilder(),
CraftDComponentKey.TEXT_VIEW_COMPONENT.key to CraftDTextBuilder(),
CraftDComponentKey.CHECK_BOX_COMPONENT.key to CraftDCheckBoxBuilder(),
CraftDComponentKey.IMAGE_COMPONENT.key to CraftDImageBuilder(),
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking... what I showed you is a good approach that works for various situations, like loading an image into an ImageView. This will be quite common when thinking about components, such as having more complex components using component composition, and you can also have an ImageView.
In this case, I think it's better to create an additional parameter in the CraftManager constructor, and then implement it within the builder (still keeping the builder as I mentioned).
like:
Stable
@Immutable
class CraftDBuilderManager (private val imageLoader : (String) -> Loader = {}){
private val mapBuilder = hashMapOf(
CraftDComponentKey.BUTTON_COMPONENT.key to CraftDButtonBuilder(),
CraftDComponentKey.TEXT_VIEW_COMPONENT.key to CraftDTextBuilder(),
CraftDComponentKey.CHECK_BOX_COMPONENT.key to CraftDCheckBoxBuilder(),
CraftDComponentKey.IMAGE_COMPONENT.key to CraftDImageBuilder(imageLoader),
)| * Supports both local and network images via Coil's AsyncImage. | ||
| */ | ||
| @Composable | ||
| fun CraftDImage( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking if we need this component here, considering a new one was created in our commonMain source set. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on the library that you will use, you can have different features os specs per platform or custom extensions.
| * | ||
| * Supports both local and network images via Coil's load extension. | ||
| */ | ||
| class CraftDImageComponent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is nice, we have it in xml too 🎉
| import androidx.compose.runtime.Composable | ||
| import androidx.compose.ui.Modifier | ||
| import androidx.compose.ui.layout.ContentScale | ||
| import coil3.compose.AsyncImage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem that this lib has support to android 100% too.
I agree with moro, i was thinking
I was thinking, I've had to do similar things using those server-driven UIs I made.
If you create a constructor in the Builder with a function that propagates to the component, like:
class CraftDImageBuilder(override val key: String = CraftDComponentKey.IMAGE_COMPONENT.key, loadImage : (String) -> Loader( algo que seja comum entre as libs teria que ver cada para pensar o objetvo certo aqui)) :
CraftDBuilder {
@Composable
override fun craft(model: SimpleProperties, listener: CraftDViewListener) {
val imageProperties = model.value.convertToElement<ImageProperties>()
imageProperties?.let {
CraftDImage(it, loadImage) { imageProperties.actionProperties?.let { listener.invoke(it) } }
}
}
}that accepts any kind of "result" from a coil or Picasso or something like that that the person implements, then when registering the components in the CraftedManager, the person could pass an implementation block using
and in the component there would be something inside the ImageView block, a loadImage.invoke(url) in Imageview
and when you register the component you can do this
val craftdBuilderManager = remember {
CraftDBuilderManager().add(
MySampleButtonComposeBuilder(),
CraftdIMageBuilder(){ url ->
//call your frameworker and return
}
)
}
LaunchedEffect(Unit) {
vm.loadProperties()
}
CraftDynamic(
properties = properties,
craftDBuilderManager = craftdBuilderManager
) {
println(
">>>> category ${it.analytics?.category} -" + " action ${it.analytics?.action} -" + " label ${it.analytics?.label} -" + " deeplink ${it.deeplink}"
)
}| import androidx.compose.runtime.Composable | ||
| import androidx.compose.ui.Modifier | ||
| import androidx.compose.ui.layout.ContentScale | ||
| import coil3.compose.AsyncImage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inside from craftdManager will be override de current CraftdImageBuilder default to new instancia
fun add(vararg arrayCraftDBuilder: CraftDBuilder) : CraftDBuilderManager{
arrayCraftDBuilder.forEach {
mapBuilder[it.key] = it
}
return this
}| import androidx.compose.runtime.Composable | ||
| import androidx.compose.ui.Modifier | ||
| import androidx.compose.ui.layout.ContentScale | ||
| import coil3.compose.AsyncImage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another thing
This PR you miss to register in CraftdManager your builder
CraftDBuilderManager
@Stable
@Immutable
class CraftDBuilderManager {
private val mapBuilder = hashMapOf(
CraftDComponentKey.BUTTON_COMPONENT.key to CraftDButtonBuilder(),
CraftDComponentKey.TEXT_VIEW_COMPONENT.key to CraftDTextBuilder(),
CraftDComponentKey.CHECK_BOX_COMPONENT.key to CraftDCheckBoxBuilder(),
CraftDComponentKey.IMAGE_COMPONENT.key to CraftDImageBuilder(),
)| kotlinx-collections-immutable = { module = "org.jetbrains.kotlinx:kotlinx-collections-immutable", version.ref = "kotlinx-collections-immutable" } | ||
|
|
||
| # Coil | ||
| coil-compose = { module = "io.coil-kt.coil3:coil-compose", version.ref = "coil" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after my comment you can remove this part
| import com.github.codandotv.craftd.androidcore.presentation.CraftDViewListener | ||
| import com.github.codandotv.craftd.xml.ui.CraftDViewRenderer | ||
|
|
||
| class ImageComponentRender(override var onClickListener: CraftDViewListener?) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to register here too
object CraftDBuilderManager {
fun getBuilderRenders(
simpleProperties: List<SimpleProperties>,
customDynamicBuilderList: List<CraftDViewRenderer<*>> = emptyList(),
onAction: CraftDViewListener
): List<CraftDViewRenderer<*>> {
val allViewRenders = (customDynamicBuilderList + listOf(
CraftDTextViewComponentRender(onAction),
ButtonComponentRender(onAction),
ImageComponentRender(onAction)
))
| import androidx.compose.runtime.Composable | ||
| import androidx.compose.ui.Modifier | ||
| import androidx.compose.ui.layout.ContentScale | ||
| import coil3.compose.AsyncImage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking... what I showed you is a good approach that works for various situations, like loading an image into an ImageView. This will be quite common when thinking about components, such as having more complex components using component composition, and you can also have an ImageView.
In this case, I think it's better to create an additional parameter in the CraftManager constructor, and then implement it within the builder (still keeping the builder as I mentioned).
like:
Stable
@Immutable
class CraftDBuilderManager (private val imageLoader : (String) -> Loader = {}){
private val mapBuilder = hashMapOf(
CraftDComponentKey.BUTTON_COMPONENT.key to CraftDButtonBuilder(),
CraftDComponentKey.TEXT_VIEW_COMPONENT.key to CraftDTextBuilder(),
CraftDComponentKey.CHECK_BOX_COMPONENT.key to CraftDCheckBoxBuilder(),
CraftDComponentKey.IMAGE_COMPONENT.key to CraftDImageBuilder(imageLoader),
)|
I will take some time tomorrow to fix this, thanks guys, for the suggestions |
Description
Added Coil 3 image component (CraftDImage + builder) and updated Gradle files to include coil-compose and coil‑network‑okhttp dependencies. Updated version catalog and cleaned up build scripts.