-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Image handling improvements #72
Conversation
On a side note, while testing this I've created a sample project with multiplatform support(Android, JVM and IOS). We can also merge that here if it adds any value. |
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 PR looks great! Haven't had time to go into full depth yet, but overall this looks really promising.
Thank you so much for opening the PR!
Added a few comments and questions
* Local [ImageTransformer] provider | ||
*/ | ||
val LocalImageTransformer = staticCompositionLocalOf<ImageTransformer> { | ||
error("No local ImageLinkTransformer") |
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.
No local ImageTransformer
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.
We can provide the ImageTransformerImpl
here and remove the imageTransformer
parameter from Markdown() composable but doing that if the user wants to use a different imagetransformer they have to use provide the imageTransformer
using composition local provider in their code. There is noting wrong with it but I think providing imageTransformer
as a parameter is more intuitive.
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.
Oh I believe you missunderstood me. I only meant the error
message. It states: ImageLinkTransformer
instead of ImageTransformer
(the comment was basically repeating the error message but in fixed form :D)
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.
sorry my bad, I had named it ImageLinkTransformer
initially.
import com.mikepenz.markdown.utils.imagePainter | ||
import com.mikepenz.markdown.utils.painterIntrinsicSize | ||
|
||
internal object ImageTransformerImpl : ImageTransformer { |
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 believe it would probably better to make this a normal class instead of an object. As we won't need it to exist if somebody provides their own implementation.
import androidx.compose.ui.graphics.painter.Painter | ||
|
||
@Composable | ||
internal actual fun imagePainter(url: String): Painter? { | ||
return null | ||
return null as Painter? // just `return null` crashes on native |
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.
Thank you!
if (parentSize.isUnspecified) { | ||
TextUnitSize(180.sp, 180.sp) | ||
} else if (intrinsicImageSize.isUnspecified) { | ||
TextUnitSize(parentSize.width.toSp(), 180.sp) | ||
} else { |
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.
Using TextSize
for an image? Would it maybe make more sense to have this exposed as normal IntSize
instead? 🤔
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 made this sp size class because the Placeholder needs sp dimensions and doing conversion here is better. I might be able to remove it. Let's see.
Once again thank you so much for this PR! |
We can use a different image loader to load the images. Here is a snippet using Kamel to load the images