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

Fix issue with svg images starting not with a <svg> tag #3789

Merged
merged 2 commits into from Sep 14, 2021

Conversation

raethlein
Copy link
Collaborator

Issue: #3655

Description: The current check whether an svg file starts with <svg...> is replaced with a regex. The regex also the svg file to start also with a <xml...> tag and other optional tags (e.g. <!DOCTYPE...>) before the <svg...> appears which fixes the linked issue. This is not a fully-fledged svg-validation, though.


Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@raethlein raethlein changed the title Replace check of string start char with regex Fix issue with svg images starting not with a <svg> tag Sep 13, 2021
Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

The fix itself LGTM, but I also think we'll want to add a test or two to the e2e tests for st.image to ensure that things are working as expected (I think there are already some tests displaying svg strings, so showing those same strings again with some added optional tags should be sufficient).

You can find the relevant test files in
e2e/scripts/st_image.py and e2e/specs/st_image.spec.js.

@raethlein
Copy link
Collaborator Author

Thanks @vdonato for the quick review and you are totally right, thanks for pointing it out!
I added some tests to the other svg tests, also to lib/tests/streamlit/image_test.py. What do you think?

Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

LGTM!

@raethlein raethlein merged commit 66a3a04 into streamlit:develop Sep 14, 2021
@raethlein raethlein deleted the bugfix/3655 branch September 14, 2021 20:28
tconkling added a commit to tconkling/streamlit that referenced this pull request Sep 15, 2021
* develop:
  Upgrade Mypy (streamlit#3797)
  Updated padding for radio button (streamlit#3377) (streamlit#3784)
  Fix issue with svg images starting not with a <svg> tag (streamlit#3789)
  Bump axios from 0.21.1 to 0.21.4 in /frontend (streamlit#3794)
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.

None yet

2 participants