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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade react-markdown to version 6 #4221

Merged
merged 16 commits into from Jan 14, 2022

Conversation

kmcgrady
Copy link
Collaborator

@kmcgrady kmcgrady commented Dec 28, 2021

馃摎 Context

This resolves some issues with markdown. The upgrades introduced
some breaking changes, so the changes have been made. Specifically:

  • Code Blocks were updated to the new Components standard
  • Installation of rehype plugins for math and raw HTML

NOTE: Specific upgrade to version 6 over version 7. Version 7 forces us onto ESM. I think it's possible but requires much more refactor.

  • What kind of change does this PR introduce?

    • Refactoring

馃И Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

馃寪 References


Contribution License Agreement

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

Ken McGrady added 3 commits December 28, 2021 09:42
This resolves some issues with markdown. The upgrades introduced
some breaking changes, so the changes have been made
@kmcgrady kmcgrady changed the title Upgrade react-markdown to version 7 Upgrade react-markdown to version 6 Dec 28, 2021
@kmcgrady kmcgrady closed this Dec 29, 2021
@kmcgrady kmcgrady reopened this Dec 29, 2021
@stale
Copy link

stale bot commented Jan 13, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 13, 2022
@kmcgrady kmcgrady removed the stale label Jan 13, 2022
@kmcgrady kmcgrady marked this pull request as ready for review January 13, 2022 21:41
Copy link
Collaborator

@mayagbarnes mayagbarnes left a comment

Choose a reason for hiding this comment

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

Looks good to me! 馃槃

Admittedly, I still find the updated snapshots for caption-3.snap.png & caption-3-dark.snap.png confusing... if I'm understanding, the vertical gap between "This is a caption that contains" and "html" is bc of the <div> tag... but I would think the intended behavior is for the word "html" to be rendered within the <div> tag like in the original screenshot.

@kmcgrady
Copy link
Collaborator Author

kmcgrady commented Jan 14, 2022

Yea. The main reason is that When markdown is written in plain text (no header, etc) it's assumed to be in a <p> tag, which I think is correct. The interesting thing, is the <div> appears which breaks the <p> and as a result ends the paragraph, inserts the <div> and continues the text after it. This would be different if it were some inline tag like <strong>. At this point, the Markdown is trying to figure out what to do, and chose this option, which is not unreasonable.

the word "html" remains in the <div> tag.

See the PR preview and you can see when you select the caption test https://share.streamlit.io/streamlit/core-previews/pr-4221

@mayagbarnes
Copy link
Collaborator

Gotcha! And spacing makes more sense now, it isn't coming from a blank <div> like I thought but rather the <p> tag's bottom margin. I should probably adjust that test at some point 馃槄

@kmcgrady kmcgrady merged commit be79768 into streamlit:develop Jan 14, 2022
@kmcgrady kmcgrady deleted the refactor/react-markdown branch January 14, 2022 23:28
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