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

Improved notebook cell drag images #13791

Merged
merged 6 commits into from
Jun 13, 2024

Conversation

jonah-iden
Copy link
Contributor

What it does

This improves the rendering of drag images for notebook cells and aligns to more how they look in vscode.

How to test

Open notebook and try dragging a cell.
Code and mardown cells have different drag image renderers

Follow-ups

Review checklist

Reminder for reviewers

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@jonah-iden jonah-iden requested a review from msujew June 10, 2024 15:02
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I'm wondering whether we should call NotebookCellListView.dragGhost?.remove() on onDrop as well. Otherwise the element stays in the body indefinitely, right?

Aside from that, looks pretty great. Good idea using the markdown renderer to produce "pseudo" editors during dragging.

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@jonah-iden jonah-iden requested a review from msujew June 11, 2024 06:52
@jonah-iden jonah-iden changed the title IMproved notebook cell drag images Improved notebook cell drag images Jun 11, 2024
…outside of the notebook editor

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I've managed to break the markdown rendering by including ``` in the first line of the cell source:

image

I think it would be alright to just run replaceAll('```', '') on the lines value. It probably doesn't matter if it had any significance in the input text.

Aside from that, I've noticed that scrolling in the notebook doesn't work while dragging a cell, but that's unrelated to the PR.

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
@jonah-iden
Copy link
Contributor Author

@msujew thanks, good catch. fixed it the way you suggested for. Not super happy with that though since in python iirc you use ``` to indicate multi line strings.

The scrolling seems to be general theia problem. Same thing happens in the file explorer and other places as soon as you drag anything. If there is not already an issue for that we should probably reate one

@msujew
Copy link
Member

msujew commented Jun 11, 2024

@jonah-iden AFAIK python uses """. Anyway, I found a way to correctly escape everything and pushed it to your branch.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks 👍

@jonah-iden jonah-iden merged commit fdfb59d into master Jun 13, 2024
14 checks passed
@jonah-iden jonah-iden deleted the jiden/improved-notebook-cell-drag-images branch June 13, 2024 07:51
@github-actions github-actions bot added this to the 1.51.0 milestone Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants