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: run markdown cells synchronously before hiding code #3385

Closed
wants to merge 2 commits into from

Conversation

metaboulie
Copy link
Contributor

@metaboulie metaboulie commented Jan 9, 2025

📝 Summary

reported here
https://discord.com/channels/1059888774789730424/1326825858337607704

This ensures markdown cells are properly rendered before their code is hidden from view, while maintaining a smoother user experience.

Before

Screen.Recording.2025-01-09.at.20.15.03.mov

Now

Screen.Recording.2025-01-09.at.20.13.30.mov

📋 Checklist

  • I have read the contributor guidelines.
  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • I have added tests for the changes made.
  • I have run the code and verified that it works as expected.

📜 Reviewers

@akshayka OR @mscolnick

Copy link

vercel bot commented Jan 9, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marimo-docs 🛑 Canceled (Inspect) Jan 9, 2025 0:20am
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 9, 2025 0:20am

Copy link

@filippo-orru filippo-orru left a comment

Choose a reason for hiding this comment

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

Thanks for the quick response! At first I thought this might not fix the problem I reported, because this is only about markdown cells, but I used python: mo.md("# heading").
But it seems like new versions of marimo automatically treat these "pure" markdown cells just like cells created using the add markdown cell button.

Looks good to me!

Copy link
Contributor

@mscolnick mscolnick left a comment

Choose a reason for hiding this comment

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

nice change!

// For markdown cells, ensure the cell is run before hiding
if (nextHidden && languageAdapter === "markdown") {
runCell();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @Light2Dark @akshayka for visibility, this looks good to me but want to get one of your stamps first

Copy link
Contributor

@Light2Dark Light2Dark Jan 9, 2025

Choose a reason for hiding this comment

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

I thought this was fixed here: #3320 🤔. This would run the cell even if it's not meant to be run I think. I'm okay with it if that's an acceptable behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like there's an edge case that #3320 doesn't catch?

I don't really mind running pure markdown cells preemptively. But I wonder if a proper fix would figure out what was missing from #3320 and address that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could I ask what the edge case is @metaboulie ? I'm struggling to reproduce this issue in the latest pull

Screenshot 2025-01-10 at 1 06 15 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Light2Dark Yeah, #3320 should have fixed this problem, thanks 🙂

@metaboulie metaboulie closed this Jan 10, 2025
@metaboulie metaboulie deleted the hidden-cell branch January 10, 2025 02:02
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.

5 participants