repl: Streamline Markdown output usage#47713
Conversation
bc8d0a7 to
5962985
Compare
MrSubidubi
left a comment
There was a problem hiding this comment.
Thank you again for pushing this forward and helping with this. Left two minor notes
crates/repl/src/outputs/markdown.rs
Outdated
| ) | ||
| })) | ||
| .into_any_element() | ||
| MarkdownStyle { |
There was a problem hiding this comment.
I think instead of duplicating this, I'd probably prefer if we were to move the default_markdown_style from the agent_ui into the markdown crate perhaps (and probably make it an associated function of MarkdownStyle even), given that we want to style these the same way anyway. What do you think?
There was a problem hiding this comment.
There is some slight difference as default_markdown_style in the agent_ui crate uses agent_buffer_font_size and agent_ui_font_size from the theme settings. I generally agree with you though as I'd love to have markdown text content be consistent.
There was a problem hiding this comment.
Didn't notice that, thanks for mentioning it.
I took another look just now, and it might be worth to traitify fonts across the code base at some point, although that is very much a chore work I neither require nor want you to do (after all, thanks for helping here).
However, as an intermediate solution: How would you feel about moving it and have the default_markdown_style take an enum instead that covers the current 4 font types, like
enum MarkdownFont {
AgentBuffer, AgentUi, Buffer, UI }for the time being?
There was a problem hiding this comment.
Yeah I can do that.
b376737 to
05d7682
Compare
Migrate from legacy markdown_preview to modern markdown crate which has proper text wrapping support and better styling capabilities. Fix width constraints in output rendering: - Add overflow_x_hidden to output area flex container to enforce bounds - Remove size_full from execution view container (conflicts with flex_1) - Add proper width constraints to output flex children - Make horizontal scrolling conditional (only tables/images need it) Update notebook cells to use modern markdown rendering with reset() method. The modern markdown crate uses proper text layout with WhiteSpace::Normal by default and respects width constraints when overflow is hidden.
df8b875 to
59af71b
Compare
|
Ok @MrSubidubi, I've extracted out the
I opted to drop the leading |
MrSubidubi
left a comment
There was a problem hiding this comment.
This is awesome, I love every piece of it! Reads so much better at a glance and love that we do not have to duplicate the style anymore
Looks good to me, thanks again and thank you for the quick turnarounds!
|
Sadly I failed to update the release notes at the top in time. I switched them to N/A when I was in a draft state and unsure if I fully solved the Markdown output wrapping issue. Now it's fixed! If anything we could make it:
I'll update above but I think it'll miss automation. |
|
Yeah I think you are right about the automation part - I'll notify Joseph about it for next weeks release. |
|
Release notes are only generated after the items land in a preview branch. Also, when the script does run on the preview branch, it pings up to the pull requests on GitHub to pull the latest notes—it doesn't look at what landed in the git history. So, since this PR only just hit main today, you've got a week to adjust the notes before they are picked up by the scripts. So you're all good! |
|
Thank you @JosephTLyons! I've updated it. |
Brought the Markdown output up to date with how Markdown is used in the Agent panel. This fixed an issue with outputs that were too large for the execution view as well as made sure that markdown would wrap.
Release Notes: