Skip to content

Conversation

@dhruvisompura
Copy link
Contributor

@dhruvisompura dhruvisompura commented Dec 2, 2025

Addresses #10455

This updates markdown cells so that they:

  • show a pencil icon in action bar (instead of chevron down) to toggle edit mode
  • show a checkmark icon in action bar (instead of chevron up) to toggle view mode
  • do not show a rendered preview of the cell contents when in edit mode

AFTER

Screen.Recording.2025-12-01.at.4.10.58.PM.mov

Release Notes

New Features

  • N/A

Bug Fixes

  • N/A

QA Notes

@:positron-notebooks

@github-actions
Copy link

github-actions bot commented Dec 2, 2025

E2E Tests 🚀
This PR will run tests tagged with: @:critical @:positron-notebooks

readme  valid tags

nstrayer
nstrayer previously approved these changes Dec 3, 2025
Copy link
Contributor

@nstrayer nstrayer left a comment

Choose a reason for hiding this comment

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

Works wonderful! Just a couple code style suggestions.

Comment on lines 102 to 109
/* When selected/editing AND no outputs: restore bottom rounding */
/* When code cell is selected/editing AND no outputs: restore bottom rounding */
.positron-notebook-cell.selected:has(.positron-notebook-cell-outputs.no-outputs) .positron-notebook-editor-container::after,
.positron-notebook-cell.editing:has(.positron-notebook-cell-outputs.no-outputs) .positron-notebook-editor-container::after,
.positron-notebook-cell:not(.selected):not(.editing):hover:has(.positron-notebook-cell-outputs.no-outputs) .positron-notebook-editor-container::after {
.positron-notebook-cell:not(.selected):not(.editing):hover:has(.positron-notebook-cell-outputs.no-outputs) .positron-notebook-editor-container::after,
/* When markdown cell is in edit mode (no output shown): restore bottom rounding */
.positron-notebook-cell.selected:not(:has(.positron-notebook-cell-outputs)) .positron-notebook-editor-container::after,
.positron-notebook-cell.editing:not(:has(.positron-notebook-cell-outputs)) .positron-notebook-editor-container::after,
.positron-notebook-cell:not(.selected):not(.editing):hover:not(:has(.positron-notebook-cell-outputs)) .positron-notebook-editor-container::after {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is getting a bit crazy. I wonder if using nested syntax may make things a bit easier to reason about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed this is a bit intense. I'll update this to see if I can make it easier to read!

Comment on lines 36 to 52
{!editorShown && (
<div className='cell-contents positron-notebook-cell-outputs'>
<div
className='positron-notebook-markdown-rendered'
onDoubleClick={() => cell.toggleEditor()}
>
{
markdownString.length > 0 ?
<Markdown content={markdownString} />
: <div className='empty-output-msg'>
{emptyMarkdownCell}
{doubleClickToEdit}
</div>
}
</div>
</div>
</div>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we standardize on using just a ternary or the && short circuiting? Small preference for ternary due to risks of truthy eval in js.

seeM
seeM previously approved these changes Dec 4, 2025
Copy link
Contributor

@seeM seeM left a comment

Choose a reason for hiding this comment

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

I like this!

@dhruvisompura dhruvisompura dismissed stale reviews from seeM and nstrayer via 20ddf28 December 4, 2025 20:50
@dhruvisompura dhruvisompura merged commit cad322b into main Dec 4, 2025
12 checks passed
@dhruvisompura dhruvisompura deleted the notebooks/update-edit-icons branch December 4, 2025 23:35
@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants