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

Tools: Resize images in markdown #64

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

codeallthethingz
Copy link
Contributor

@codeallthethingz codeallthethingz commented Nov 23, 2024

This PR adds the functionality to add style attributes to images by using the fragment part of a markdown image URL.

For example

![](image.png#width=20px&height=20px)

Will result in

<img src="image.png" style="width:20px; height:20px" />

@codeallthethingz codeallthethingz added infrastructure Changes to infrastructure triaged This issue or pull request was triaged labels Nov 23, 2024
@tristanls
Copy link
Contributor

issue As we are adding a fragment to the URL, I think it should follow the usual URL convention of key=value&key=value, instead of key:value. This way, it can be easily parsed by a library like urllib.parse.parse_qs.

Copy link
Contributor

@tristanls tristanls left a comment

Choose a reason for hiding this comment

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

Thank you for the fragment format update 🙌 .

@@ -45,6 +45,5 @@ We are excited to have you here! Our intention for making the project open-sour
| <a href="https://thousandbrains.discourse.group/" style="display: flex; align-items: center;"><img src="../figures/overview/discourse.png" alt="Forum" height="15" style="opacity: 1; transition: opacity 0.2s; &:hover { opacity: 0.8; }" pointer-events="none">&nbsp;Community&nbsp;Forum</a> | Discuss ideas, ask questions and connect with other community members |
| <a href="https://thousandbrains.org/" style="display: flex; align-items: center;"><img src="../figures/overview/website.png" alt="Website" height="15" style="opacity: 1; transition: opacity 0.2s; &:hover { opacity: 0.8; }" pointer-events="none">&nbsp;Official&nbsp;Website</a> | Learn about our mission, team and the science behind the project |
| <a href="https://www.youtube.com/@thousandbrainsproject" style="display: flex; align-items: center;"><img src="../figures/overview/youtube.png" alt="YouTube" height="15" style="opacity: 1; transition: opacity 0.2s; &:hover { opacity: 0.8; }" pointer-events="none">&nbsp;YouTube&nbsp;Channel</a> | Watch tutorials, technical deep-dives and project updates |
| <a href="https://bsky.app/profile/1000brainsproj.bsky.social" style="display: flex; align-items: center;"><img src="../figures/overview/bluesky.png" alt="Bluesky" height="15" style="opacity: 1; transition: opacity 0.2s; &:hover { opacity: 0.8; }" pointer-events="none">&nbsp;Bluesky</a>| Get the latest news and announcements, and engage with our community |
|<a href="https://x.com/1000brainsproj" style="display: flex; align-items: center;"><img src="../figures/overview/twitter.png" alt="Twitter" height="15" style="opacity: 1; transition: opacity 0.2s; &:hover { opacity: 0.8; }" pointer-events="none">&nbsp;Twitter/X</a> | Get the latest news and announcements, and engage with our community |
| <a href="https://bsky.app/profile/1000brainsproj.bsky.social" style="display: flex; align-items: center;"><img src="../figures/overview/bluesky.png" alt="Bluesky" height="15" style="opacity: 1; transition: opacity 0.2s; &:hover { opacity: 0.8; }" pointer-events="none">&nbsp;Bluesky</a> <a href="https://x.com/1000brainsproj" style="display: flex; align-items: center;"><img src="../figures/overview/twitter.png" alt="Twitter" height="15" style="opacity: 1; transition: opacity 0.2s; &:hover { opacity: 0.8; }" pointer-events="none">&nbsp;Twitter</a> | Get the latest news and announcements, and engage with our community |
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): I think it should be called X here.

clean_src = src_parts[0]
style = "border-radius: 8px;"

# Parse and sanitize style parameters from fragment
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (blocking): I would like to see two things that reinforce each other: a sanitization library and an allowlist of supported keys.

Please look at the nh3 sanitization library to sanitize input: https://nh3.readthedocs.io/en/latest/#. I don't want us to maintain our own sanitization code. I would rather rely on the expertise of people who care about this sort of thing.

After the CSS keys are extracted, they should be checked against an allowlist of keys, in this case, consisting of a single one: width. However, I noticed height in the tests. Whatever the list, I think we should enumerate an allowlist to vastly reduce what can be put into the HTML.

One way to accomplish the above would be to first filter key, values for the allowlist of keys, i.e., width. Then, second, naively and unsafely construct the final HTML <figure><img... and then put the final HTML through the nh3.clean() before returning it.

tools/github_readme_sync/readme.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Changes to infrastructure triaged This issue or pull request was triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants