zed icon indicating copy to clipboard operation
zed copied to clipboard

Markdown preview action

Open kierangilliam opened this issue 1 year ago • 11 comments

This PR implements a feature I was missing from VSCode: the ability to preview markdown files inside the editor. This allows users to preview README's and other markdown documents in a rich-text format instead of plain markdown.

Changes

  • New crates/markdown_preview crate that registers the markdown: open preview action.
  • crates/markdown_preview uses crates/rich_text to convert the active editor's text into RichText. Note that markdown features not supported by RichText will also not be supported by this new action. See the below section "Missing features in rich_text".

Release Notes

  • Added Markdown: Open preview action to partially close (#6789).

https://github.com/zed-industries/zed/assets/18583882/6c1a28fc-31f6-4294-a902-94cc7ec74448

Missing features in rich_text

Needed rich_text improvements:

  • [ ] Support inline code blocks (my_code)
  • [ ] Make code blocks ~display-block instead of ~inline
  • [ ] Different heading levels should have different font sizes
  • [ ] Support task lists (- [ ] and - [x])
image

kierangilliam avatar Jan 27 '24 17:01 kierangilliam

Looks great🥳

d1y avatar Jan 27 '24 17:01 d1y

This is fantastic! We've been needing Markdown preview for a long time.

Did a quick test - exciting! One thing to note, checkmarks don't seem to be rendering in tasks:

SCR-20240127-lzcy

JosephTLyons avatar Jan 27 '24 18:01 JosephTLyons

@JosephTLyons that's correct; I'll update the PR description to reflect that. Anything that isn't implemented in crates/rich_text won't be visible in the markdown preview. What's Zed's preferred approach to shipping features like this? Land and expand? Or is this something you'd want taken care of before considering merging?

kierangilliam avatar Jan 27 '24 18:01 kierangilliam

Ok, it was a pre-existing issue here - gotcha. And I'm not sure, we've taken both approaches in the past:

  • Polishing heavily before release
  • Releasing with the expectation of following with some polish

Not sure what the right answer is here. I'll let @maxbrunsfeld or @mikayla-maki weigh in...

JosephTLyons avatar Jan 27 '24 18:01 JosephTLyons

We’re still collectively working out what it means to open source and how to interface with the community, so consider everything I say to be v0.1 and subject to change 😄

Generally, our policy internally at Zed has been to merge things once they’ve passed our collective quality bar and to prefer scrapping or postponing a feature if we can’t do it right. Right meaning fast, complete, and with a buttery smooth UI/UX experience. We don’t want to leave a lot of half baked features lying around in the source waiting for someone to remember that they exist. Obviously, we’re not always 100% on this value and some features are too large to ship in a single PR. But what we merge should never break main for our stable users and we strive to uphold that to the best of our ability at the time.

Under this policy, I’d say that the scrolling and live updating features are essential to shipping this. And further I’d say we need to figure out the UX implications of how we actually show the preview. Right now it’s implemented as its own item (tab), but I think it might be more useful as a split view? But then how does that feel with an already split view? I feel like we’d want to create a ‘SplitItem’ struct that can contain two ‘Item’s under it, so that the preview and the editor view are treated as the single document they actually are. Getting this right would also block shipping this PR.

Checkboxes are a bit of an edge case here. IMO, we could ship without them because the back-and-forth editing implied by checking them sounds like a lot more complexity and it doesn’t significantly add to the value of having markdown previews.

I hope that clarifies where my head is at, though if Max has different opinions I’d defer to his.

mikayla-maki avatar Jan 27 '24 18:01 mikayla-maki

Thanks for clarifying @mikayla-maki!

I took care of my final TODOs: scrolling and live updating. I'm at a stopping point with what I originally needed out of a markdown preview.

Since you mentioned the UX around opening the preview is unclear, I'll leave this PR in draft for now. Feel free to close it if your team decides to move in a different direction with this feature.

I appreciate the team's quick feedback and look forward to having this feature (whether or not it comes in from this PR!)

kierangilliam avatar Jan 27 '24 20:01 kierangilliam

Thank you for this PR @kierangilliam, if you or someone else wants to continue to adding the SplitItem struct I mentioned (or some other approach) we'd be happy to chat about it at one of our fireside hacks, or on a channel, or on discord or some such. For right now though, we'll check in about what to do here during our work week. Maybe I'll just implement that onto main and then you can use it for this PR 🤔. Regardless, thank you for adding this feature!

mikayla-maki avatar Jan 27 '24 20:01 mikayla-maki

@kierangilliam : The images and tables are displayed correctly?

Angelk90 avatar Jan 27 '24 21:01 Angelk90

@Angelk90

I carried out some tests with tables and images. In the tests carried out, it did not work.

I believe that for a V1 it is ok.

What do you think about this improvement coming after his PR?

Test with image:

Screenshot 2024-01-28 at 14 18 21

Test with table:

Screenshot 2024-01-28 at 14 17 52

juninhopo avatar Jan 28 '24 17:01 juninhopo

@juninhopo @Angelk90

I'm working through an alternate implementation here (6958) which has basic support for tables. This markdown renderer will also make it easier to support images in the future.

kierangilliam avatar Jan 29 '24 02:01 kierangilliam

I attempted to add the SplitItem struct I described, and it turned out to be a bit more complicated than I expected. I think this should be fine to ship without it :)

mikayla-maki avatar Jan 29 '24 17:01 mikayla-maki

This PR seems to have been superseded by https://github.com/zed-industries/zed/pull/6958

mikayla-maki avatar Jan 30 '24 02:01 mikayla-maki