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

feat: enable toggling to md mode automatically via mod-shift-m #1021

Merged
merged 2 commits into from
Mar 29, 2024

Conversation

riyavsinha
Copy link
Contributor

Features demonstrated in video:

  • markdown toggle in cell menu
  • markdown toggle doesnt show in cell menu when non-markdown-convertible code
  • markdown shortcut works (mod-shift-m)

However, it seems that when a cell is created, the editorView is null until something is typed. For this reason, I only know how to get this working after typing some whitespace character first, not sure how to actually initialize the EditorView.

Fixes #1003

marimo-md.mov

Copy link

vercel bot commented Mar 29, 2024

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

Name Status Preview Comments Updated (UTC)
marimo-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 29, 2024 7:46am
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 29, 2024 7:46am

Copy link

github-actions bot commented Mar 29, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@riyavsinha
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@riyavsinha
Copy link
Contributor Author

recheck

@akshayka
Copy link
Contributor

Awesome! Thanks so much for the PR @riyavsinha! 🙌 I really like this.

However, it seems that when a cell is created, the editorView is null until something is typed.

Weirdly, for me (on Linux), the hotkey and action button are working even on empty cells. I can't repro your issue.

I did find a different bug: when I toggle the markdown view, either through the context menu or the hotkey, the tooltip in the top right of the cell (the little "M" button) still displays "View as Markdown", whereas it should be updated to "View as Python".

markdown-button.webm

P.S. If you're on our Discord (https://discord.gg/JE7nhX6mD8), let me know what your username is and I can give you the contributor role/flair :)

Copy link
Contributor

@akshayka akshayka left a comment

Choose a reason for hiding this comment

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

Just some small comments, in addition to a comment on the PR thread.

frontend/src/core/hotkeys/hotkeys.ts Outdated Show resolved Hide resolved
frontend/src/core/codemirror/format.ts Outdated Show resolved Hide resolved
@riyavsinha
Copy link
Contributor Author

I did find a different bug: when I toggle the markdown view, either through the context menu or the hotkey, the tooltip in the top right of the cell (the little "M" button) still displays "View as Markdown", whereas it should be updated to "View as Python".

Ahh, good find -- I think this is because the LanguageToggle only initializes its state using editorView, but then doesn't rerender when the editorView languageAdapterState is changed?:

const [languageAdapter, setLanguageAdapter] = useState(
    editorView.state.field(languageAdapterState).type,
  );

I'm not sure how to get around this, since passing in language as a string prop didn't work I think (maybe because the parent ran into the same issue). useEffect also didn't do the trick using the editorView.state.field(languageAdapterState).type as a dependency. Thoughts welcome!

@mscolnick
Copy link
Contributor

@riyavsinha this is a bug on me - i have 2 places where i keep the state and since it was one callsite, i figured it was ok. ill fix this on the main to have one source of truth

@riyavsinha
Copy link
Contributor Author

riyavsinha commented Mar 29, 2024

Thank you both for the quick feedback and help so far! @mscolnick your PR worked perfectly when I pulled your changes in locally :)

The last issue with this PR is that I still get editorView = null when trying to toggle markdown using the cell context menu, but somehow when using the hotkey, editorView is not null. Would either of you happen to know why?

In the below video, I just have it print the value for editorView in toggleMarkdown() in format.ts

marimo-md-toggle-bug.mov

@akshayka
Copy link
Contributor

The last issue with this PR is that I still get editorView = null when trying to toggle markdown using the cell context menu, but somehow when using the hotkey, editorView is not null. Would either of you happen to know why?

Off the top of my head, not sure, but I can look into it

@akshayka
Copy link
Contributor

@riyavsinha, I've merged a fix onto main. With that fix, your PR should work! Thanks so much for adding for making this contribution :)

@akshayka akshayka merged commit 81bdddb into marimo-team:main Mar 29, 2024
26 checks passed
Copy link

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.3.8-dev5

Benni-Math pushed a commit to Benni-Math/marimo that referenced this pull request Apr 16, 2024
…o-team#1021)

* feat: enable toggling to md mode automatically via mod-shift-m

* feat: add reverse toggle out of markdown
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.

Markdown Cell Shortcut?
3 participants