-
-
Notifications
You must be signed in to change notification settings - Fork 116
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
Update embedding Reddit preview links to include captions and support i.redd.it embeds #92
Merged
Merged
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
6d83b07
Update embedding Reddit preview links to include captions where appli…
ButteredCats 2c8f5a7
Make figure margins only apply to comments to bring embedded previews…
ButteredCats e581f43
Use substring instead of .remove and .pop, change image_text to image…
ButteredCats 3f863c8
Prevent panic if image_caption is empty, don't replace <p>'s in case …
ButteredCats 6484ebf
Fix failing check
ButteredCats 50fad93
Fix infinite loop when replacing text that contains dollar signs
ButteredCats 75b0149
Remove useless replace
ButteredCats b22fb7c
Fix embedded preview images having a gap from the top of a comment
ButteredCats 2f2cded
Make sure new system can handle both normal and external previews
ButteredCats e9af28b
Make sure that the extra <p></p> at the bottom of a comment containin…
ButteredCats 62b791b
Add in support for embedding i.redd.it images and gifs and remove lef…
ButteredCats 3469235
Merge branch 'redlib-org:main' into fix_preview_captions
ButteredCats File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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.
Not sure exactly what this does but it should probably operate on a substring like
image_text[1..image_text.len()-4]
- if what you intend to do is remove the last 4 and first. Curious why this is done like this - can you think of a better way to do 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.
The first character of
image_text
is a>
, and could stay, but it would require readding the.replace(">>", ">")
toimage_to_replace
's assignment sinceimage_url
also contains a>
at the end of it meaning we end up with>>
in between{image_url}
and{image_text}
instead of just a single>
. I figured just removing the first character is better than theoretically having the potential to replace an intentional >> somewhere else inside ofimage_text
and having the replacement fail.The last four need to be removed since they'll be an
</a>
, which we don't want appearing inside of the<figcaption>
block. Although now that I think about it that can just be done within the if block since it isn't needed otherwise.I'll modify it to use the substring instead and only drop the
</a>
when needed.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.
Just made it use the substring as well as renaming
image_text
toimage_caption
and adding comments to make thing's functions and usages clearer.I didn't make it so that the
</a>
is only dropped whenimage_caption
is actually used to add a caption to the image as it seemed like an unnecessary extra step when it can be done at the same time as removing the first character.However I did make it so that the quotes inside
image_caption
are only replaced when it's actually used as a caption since it wasn't needed otherwise.