Skip to content
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

Markdown renderer replacement #432

Merged
merged 6 commits into from
Jun 8, 2023

Conversation

AntmanLFEz
Copy link
Contributor

Images show at approx 45% size in the markdown renderer.

This allows us to remove the markdown scaling applied to the editor which forced the text content to also be increased which made it inconsistent with the rest of the app.

Removed the compose-markdown package and created our own wrapper around the markwon libraries with a custom resolver for images.

#373

This allows us to remove the markdown scaling applied to the editor which forced the text content to also be increased.

Removed the compose-markdown package and created our own wrapper around the markwon libraries with a custom resolver for images.

LemmyNet#373
@AntmanLFEz AntmanLFEz requested a review from dessalines as a code owner June 5, 2023 23:54
@twizmwazin
Copy link
Contributor

I tested this out and it seems to work quite well. Text size is correct, and images render correctly and don't cause weird resizing and snapping issues I was experiencing with the previous renderer.

@twizmwazin
Copy link
Contributor

twizmwazin commented Jun 6, 2023

Some follow up notes from testing:

  • If the last character of a line is italicized, it will be cut off. For example, if a comment is deleted, the d will appear as an a.
  • Images are unloaded if they go off screen
  • Images sharing a line with text do not have sufficient padding with that text. While this is probably correct for markdown, there should probably be some effort on the app's side to make it look better, by inserting additional newlines before and after pictures if they aren't there already.
  • Sometimes images are very small and effectively unviewable.
  • I had to update proguard rules on my local branch and I think this PR was responsible since it is the only one changing dependencies.

Here is an excellent thread for testing: https://beehaw.org/post/433016

@AntmanLFEz
Copy link
Contributor Author

Thanks for the feedback @twizmwazin

I found the issue / fix for the very small pictures. I will take a look at the other items soon

@AntmanLFEz
Copy link
Contributor Author

Actually the best option will be to use image-coil as the backend. Testing it now with my other fix seems to work very well. No image unloading.

Will do more testing and try and apply the same preview etc we use for post images.

@dessalines
Copy link
Member

Did you look at this PR at all? #274

RichText material 3 seems to be the best future-proof option for markdown rendering... I just couldn't get images working properly with it. IMO it would be better to work on that directly to add better image support, since that would positively affect all jetpack-compose projects that need markdown.

@AntmanLFEz
Copy link
Contributor Author

@dessalines I will take a look.

So far I have updated the pull request with the coil implementation. So far it works pretty well (with limited testing)

Apologies if i butchered the PR. Its my first stab at doing it "properly"

@dessalines
Copy link
Member

Its no problem, I should be able to get to this tomorrow.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing, I'd say this is already better than the current renderer, so great job! Just fix that one issue above, and we can merge.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, there's one merge conflict.

@dessalines dessalines merged commit abd1b49 into LemmyNet:main Jun 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants