You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR is a continuation of the overall MediaView refactor - this time focusing on the MediaView widget and link previews. Here are some high level notes on the changes:
Removed LinkPreviewCard and replaced its usages with MediaView. LinkPreviewCard shared a lot of related logic with MediaView and made the code harder to follow. This led to some visual inconsistencies across link previews vs. image/video previews.
Started the deprecation of scrapeMissingPreviews option - I'm not sure how useful this option has been recently. In the past, there were issues with thumbnail generation which were mostly resolved in the backend. Having this option complicates the logic a bit, and has some potential performance issues as it has to fetch and parse through HTML content to find potential thumbnails (which should ideally be done at the backend regardless). That being said, do let me know if you feel strongly against this change!
Combined shared logic in MediaView together (e.g., shared container and box constraints) - this removes duplicated logic across the image/video widgets.
Removed dependency on PostViewMedia within MediaView. MediaView now only depends on a Media rather than the full PostViewMedia. This allows the widget to be more flexible and be used in places that don't have access to PostViewMedia (e.g., creating a new post with images)
This should more or less conclude the major refactors for MediaView, although there may be some smaller additional PRs to possibly split up/simplify the logic even more (if needed). Again, I did try to do as much testing as possible but there may be some bugs that were not caught during my testing, so please mention any issues that pop up!
This PR should also fix the issues mentioned here: #1715 (comment)!
Issue Being Fixed
Issue Number: N/A
Screenshots / Recordings
Checklist
If a new package was added, did you ensure it uses an appropriate license and is actively maintained?
Did you use localized strings (and added appropriate descriptions) where applicable?
Did you add semanticLabels where applicable for accessibility?
Started the deprecation of scrapeMissingPreviews option - I'm not sure how useful this option has been recently. In the past, there were issues with thumbnail generation which were mostly resolved in the backend. Having this option complicates the logic a bit, and has some potential performance issues as it has to fetch and parse through HTML content to find potential thumbnails (which should ideally be done at the backend regardless). That being said, do let me know if you feel strongly against this change!
I'm good with this! There are definitely still plenty of cases where Lemmy fails to find a thumbnail for the post, but these issues seem to be consistently reported to them and fixed by them. Plus users now have the option to add a custom thumbnail. So I don't see any reason for us to do additional work in this area.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Description
This PR is a continuation of the overall
MediaViewrefactor - this time focusing on theMediaViewwidget and link previews. Here are some high level notes on the changes:LinkPreviewCardand replaced its usages withMediaView.LinkPreviewCardshared a lot of related logic withMediaViewand made the code harder to follow. This led to some visual inconsistencies across link previews vs. image/video previews.scrapeMissingPreviewsoption - I'm not sure how useful this option has been recently. In the past, there were issues with thumbnail generation which were mostly resolved in the backend. Having this option complicates the logic a bit, and has some potential performance issues as it has to fetch and parse through HTML content to find potential thumbnails (which should ideally be done at the backend regardless). That being said, do let me know if you feel strongly against this change!MediaViewtogether (e.g., shared container and box constraints) - this removes duplicated logic across the image/video widgets.PostViewMediawithinMediaView.MediaViewnow only depends on aMediarather than the fullPostViewMedia. This allows the widget to be more flexible and be used in places that don't have access toPostViewMedia(e.g., creating a new post with images)This should more or less conclude the major refactors for MediaView, although there may be some smaller additional PRs to possibly split up/simplify the logic even more (if needed). Again, I did try to do as much testing as possible but there may be some bugs that were not caught during my testing, so please mention any issues that pop up!
This PR should also fix the issues mentioned here: #1715 (comment)!
Issue Being Fixed
Issue Number: N/A
Screenshots / Recordings
Checklist
semanticLabels where applicable for accessibility?