Skip to content

Conversation

@nakul-py
Copy link
Contributor

@nakul-py nakul-py commented Oct 23, 2025

This Pr fixes Accept and Reject changes buttons overlapping with cell toolbar bug.
Now when the diff is active cell toolbar hides and visible when diff is deactivated.
Thanks @Zsailer for this better and clean apporoach.

Screencast.From.2025-10-25.13-35-36.mp4

CC @jtpio @Zsailer @brichet

@jtpio jtpio added the bug Something isn't working label Oct 24, 2025
style/base.css Outdated
gap: 2px;
padding: 0;
line-height: 1;
margin-top: 20px;
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, that would mean it applies to all chunks?

Do you have screenshot of what it looks like with a bigger diff?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering otherwise if we should only treat the case of the first chunk separately, so it does not overlap with the cell toolbar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtpio I have shared some screenshots.

@nakul-py
Copy link
Contributor Author

nakul-py commented Oct 24, 2025

Want screenshots like this

Screenshot From 2025-10-24 16-00-46


Screenshot From 2025-10-24 16-02-40

@nakul-py nakul-py requested a review from jtpio October 24, 2025 10:34
@jtpio
Copy link
Contributor

jtpio commented Oct 24, 2025

Thanks.

Curious how this will work when we allow inline diffs? #27

Will the buttons be "off" due to the extra margin?

@nakul-py
Copy link
Contributor Author

Its hard to say without experiencing but i also think the buttons be "off" due to the extra margin?
I think let we first make inline diffs work than we can focus on this.

@Zsailer
Copy link
Collaborator

Zsailer commented Oct 24, 2025

Hey y'all, I don't think we should be adding this spacing to every diff, just to solve the first line problem when cell buttons overlap. From an aesthetic perspective, it doesn't look right to me that these buttons aren't always flush in the very top right of the diff.

I believe a better approach—though I recognize this might be difficult to achieve—would be to disable/hide the cell tools when a diff is visible. When showing a diff, I don't think the user needs the cell tools until they are done handling the diff, so it's appropriate to hide them.

@nakul-py
Copy link
Contributor Author

I believe a better approach—though I recognize this might be difficult to achieve—would be to disable/hide the cell tools when a diff is visible. When showing a diff, I don't think the user needs the cell tools until they are done handling the diff, so it's appropriate to hide them.

Sounds Good!

@nakul-py
Copy link
Contributor Author

@jtpio and @Zsailer like this...

Screencast.From.2025-10-24.22-02-17.mp4

@Zsailer
Copy link
Collaborator

Zsailer commented Oct 26, 2025

@nakul-py yeah, this looks great! Thank you for implementing this. One more question... after accepting/rejecting the diff, does the cell toolbar come back? I want to make sure we don't permanently hide the toolbar. It should reappear as soon as the diff is handled.

@nakul-py
Copy link
Contributor Author

nakul-py commented Oct 26, 2025

Yes cell toolbar comes back when diff is completed as you can also see in

Screencast.From.2025-10-25.13-35-36.mp4

@Zsailer
Copy link
Collaborator

Zsailer commented Oct 26, 2025

LGTM! Thanks @nakul-py!

@Zsailer Zsailer merged commit 4a98f34 into jupyter-ai-contrib:main Oct 26, 2025
6 checks passed
@nakul-py nakul-py deleted the fix22 branch October 26, 2025 14:10
@jtpio jtpio mentioned this pull request Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Overlap with the cell toolbar

3 participants