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

Communication: Fix link rendering for attachments and show uploaded images in chat #109

Merged
merged 19 commits into from
Nov 30, 2024

Conversation

julian-wls
Copy link
Contributor

@julian-wls julian-wls commented Nov 15, 2024

Description

This PR fixes an issue that caused lecture attachments such as slides and lecture units to not correctly render as links. Furthermore, it changes the ProvideMarkwon composable to use a custom ImageLoader that is generated by a new class called DefaultImageProvider and is capable of handling authorization. DefaultImageProvider has been introduced to request server images and is also used for the course images now. All images that have been uploaded to the server can be displayed now.
This PR will close #61

Important side note:
Although the links to lecture attachments are displayed correctly now, users still face an authorization error and do not get access to the resource. Fixing this issue is outside the scope of this PR.

Screenshots

Screenshot1

Screenshot8

@julian-wls julian-wls self-assigned this Nov 15, 2024
@julian-wls julian-wls added the ready for review This PR can be reviewed label Nov 15, 2024
@julian-wls julian-wls requested review from FelberMartin, asliayk and TimOrtel and removed request for asliayk November 15, 2024 17:20
Copy link
Collaborator

@FelberMartin FelberMartin left a comment

Choose a reason for hiding this comment

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

Tested the functionality, slides and lecture units are displayed correctly, and clicking them navigates to the repsective part of the app. When clicking on slides I get redirected to the browser but I actually do not have to authenticate there. Maybe this is because I was already logged into the server in chrome?

Showing images in the chat works.

@FelberMartin
Copy link
Collaborator

I just noticed that when reloading the chat all messages above the chat are jittering, can you have a look at this? The shown chat is on TS0 in "$mo-1"

ArtemisChatImageReloadJitter

@TimOrtel
Copy link
Contributor

I just noticed that when reloading the chat all messages above the chat are jittering, can you have a look at this? The shown chat is on TS0 in "$mo-1"

ArtemisChatImageReloadJitter ArtemisChatImageReloadJitter

Have you verified that this problem does not occur on the develop branch? Otherwise, I would create an issue and a separate PR for it.

Copy link
Collaborator

@FelberMartin FelberMartin left a comment

Choose a reason for hiding this comment

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

When checking out the branch the reload jittering with images still persist, but tbh I think we should create a separate issue / PR for this at this point and get this PR merged, I think people are waiting for the fixes and features of this PR

@julian-wls
Copy link
Contributor Author

julian-wls commented Nov 28, 2024

When checking out the branch the reload jittering with images still persist, but tbh I think we should create a separate issue / PR for this at this point and get this PR merged, I think people are waiting for the fixes and features of this PR

I agree. I noticed that if the images are smaller than the default height, there is jittering. For images that have a greater original height, it doesn't happen.
You can see this on TS1 in the #random chat of Practical Course: Interactive Learning WS24/25. There is an image in the chat, which acts as an example for the explained behavior.

@FelberMartin FelberMartin merged commit 9b6c193 into develop Nov 30, 2024
5 checks passed
@FelberMartin FelberMartin deleted the bugfix/communication/attachments-rendering branch November 30, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge This PR can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Referencing attachment does not render properly
3 participants