improve: Hide code by default for Markdown cells #2507
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
While testing this, I noticed that I can't click the Delete button for a hidden Markdown cell anymore. The "show code / hide code" overlay keeps getting triggered instead. Tried messing around with the z-index but couldn't get it to work.
|
|
I imagined this being config driven, with new markdown cells defaulting to hide_code=True, but this may work. Let me play with it and see how it feels. |
|
Yeah we should probably just do that for behavioral consistency. |
Oh that must be a consequence of #2504 ... @wasimsandhu I have a fix for this, will create a separate PR. |
frontend/src/core/cells/cells.ts
Outdated
| @@ -241,6 +242,10 @@ const { | |||
| } | |||
| } | |||
|
|
|||
| const isMarkdown = newCellCode | |||
| ? LanguageAdapters.markdown().isSupported(newCellCode) | |||
There was a problem hiding this comment.
isSupported returns true for empty string ""., maybe we could check its not empty as well, or add a function isMarkdown
|
@akshayka Not sure why I thought this change was on the frontend only. Every new Markdown cell that is created now has |
| editorViewParentRef.current?.addEventListener( | ||
| editorViewRef.current?.contentDOM.addEventListener( |
There was a problem hiding this comment.
Hmm, looks like my fix in #2508 got reverted. Is this intentional?
There was a problem hiding this comment.
Following up with a fix, blur -> focusout
There was a problem hiding this comment.
Yes sorry, I left a comment on that PR but forgot to put one here. And thanks, that did the trick!
There was a problem hiding this comment.
Well actually, it looks like you can't delete cells with the button again... Should we just move it somewhere else?
| // For a newly created Markdown cell, which defaults to | ||
| // hidden code, we temporarily show the code editor | ||
| useEffect(() => { | ||
| if (!(isMarkdown(code) && code === `mo.md(r"""\n""")`)) { |
There was a problem hiding this comment.
can we do if (code === new MarkdownLanguageAdapter().defaultCode)
| return; | ||
| } | ||
| temporarilyShowCode(); | ||
| }, [code, temporarilyShowCode]); |
There was a problem hiding this comment.
i might be better to just remove temporarilyShowCode and add the eslint disable. clearer that it only react to code changes
This reverts commit 9a674dc. The logic for temporarily showing code for markdown cells is incorrect and causing all hidden cells to not render as hidden
## 📝 Summary <!-- Provide a concise summary of what this pull request is addressing. If this PR fixes any issues, list them here by number (e.g., Fixes #123). --> Follow up on #2507 . <img width="834" alt="Screenshot 2024-12-03 at 9 02 17 PM" src="https://github.com/user-attachments/assets/7f17749f-01aa-42ef-b2de-ee4c58c2bed5"> ## 🔍 Description of Changes <!-- Detail the specific changes made in this pull request. Explain the problem addressed and how it was resolved. If applicable, provide before and after comparisons, screenshots, or any relevant details to help reviewers understand the changes easily. --> - There were also some UI bugs from before that are fixed here. (I didn't account for Cell Output below the cell 🙏) ## 📋 Checklist - [X] I have read the [contributor guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md). - [ ] For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on [Discord](https://marimo.io/discord?ref=pr), or the community [discussions](https://github.com/marimo-team/marimo/discussions) (Please provide a link if applicable). - [ ] I have added tests for the changes made. - [X] I have run the code and verified that it works as expected. ## 📜 Reviewers <!-- Tag potential reviewers from the community or maintainers who might be interested in reviewing this pull request. Your PR will be reviewed more quickly if you can figure out the right person to tag with @ --> @akshayka OR @mscolnick
📝 Summary
Create Markdown cells on the frontend with
hide_codeset totrueby default. Closes #2389.🔍 Description of Changes
With @akshayka's change in #2504, this makes it so that the code editor for a Markdown cell is shown when editing, but hidden when the cell is not focused.
📋 Checklist