Skip to content

Conversation

@nakul-py
Copy link
Contributor

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

Adding Accept and Reject All buttons for bulk code changes.

Screencast.From.2025-10-25.22-19-13.mp4

Closes #23

CC @jtpio @Zsailer

@nakul-py nakul-py marked this pull request as ready for review October 24, 2025 12:20
@jtpio
Copy link
Contributor

jtpio commented Oct 24, 2025

Thanks for looking into this!

While they would do the job, these buttons seem to be taking a lot of space in the notebook toolbar. Also, they push the other toolbar buttons to the left when they are added, which causes a bit of a distraction.

So maybe they should be a bit smaller, or be placed somewhere else in the notebook toolbar. Another idea would be to have them somewhere "hovering" in the notebook itself.

Not sure yet which one of these different approaches would be the best in terms of UX though.

@Zsailer Zsailer added the enhancement New feature or request label Oct 24, 2025
@Zsailer
Copy link
Collaborator

Zsailer commented Oct 24, 2025

Thanks for taking this on @nakul-py!

somewhere "hovering" in the notebook itself.

Yes, I would agree with @jtpio on this approach instead. I think it should be a little pop up in the bottom right corner of the notebook—similar to a (toast) notification. We shouldn't actually use the notification UI in JupyterLab, though, since it persists when you switch notebooks. But we can use a similar UX, where a small hovering panel appears in the bottom right corner and follows the notebook panel (meaning if switch notebook panels and come back, it's still there).

@nakul-py
Copy link
Contributor Author

Hey @jtpio and @Zsailer I have Moved Both buttons to Floating state and I have also updated PRs Screen recording can you have a look on it?

@nakul-py nakul-py requested a review from Zsailer October 26, 2025 09:01
@nakul-py
Copy link
Contributor Author

@Zsailer and @jtpio This is how Buttons looks now

Screencast.From.2025-10-26.14-25-27.mp4

Any other suggestion Or i have a question is that we should see Accept All, Reject All buttons in toolbar with one cell change or only when more then one cell change?

Because one cell changes can be done by init buttons.

And what about if user accept changes within cell buttons so we have to hide our button floating panel which can be done in follow up PR.

@Zsailer
Copy link
Collaborator

Zsailer commented Oct 26, 2025

@nakul-py we're getting closer! Thank you for making these changes!

Okay, I'm going to be super annoying here again 😅 – really sorry for the additional minor styling requests...

Your video helped me spot a couple more alignment issues:

  • Corner radius: The popup corners should actually go to zero to match the tab-complete modal (which has no radius for consistency)
  • Button corners: Should be reduced to match other UI buttons – looks like the "+" button in the file browser uses ~4px radius
  • Button text: Should remain white in both light and dark modes (the dark text is hard to read)

Thanks for being patient with my pixel-pushing! 🙏

@Zsailer
Copy link
Collaborator

Zsailer commented Oct 26, 2025

we should see Accept All, Reject All buttons in toolbar with one cell change or only when more then one cell change

Probably not. It should only appear if multiple cells have been changed.

what about if user accept changes within cell buttons so we have to hide our button floating panel which can be done in follow up PR.

That's right. This button should be responsive to actions taken within the cells. If the user addresses the diffs directly in the cells, once they get to their last cell, this modal should go away on it's own.

@nakul-py
Copy link
Contributor Author

we should see Accept All, Reject All buttons in toolbar with one cell change or only when more then one cell change

Probably not. It should only appear if multiple cells have been changed.

what about if user accept changes within cell buttons so we have to hide our button floating panel which can be done in follow up PR.

That's right. This button should be responsive to actions taken within the cells. If the user addresses the diffs directly in the cells, once they get to their last cell, this modal should go away on it's own.

OK! i will done these in separate PRs. :)

@nakul-py
Copy link
Contributor Author

@Zsailer Is this looks OK now?

Screenshot From 2025-10-26 19-37-08

@Zsailer
Copy link
Collaborator

Zsailer commented Oct 26, 2025

Yes, this looks great to me! Thank you!

@nakul-py
Copy link
Contributor Author

Yes, this looks great to me! Thank you!

Ok then i am pushing the commit :)

@nakul-py
Copy link
Contributor Author

Why these checks were failing now 🤷‍♂️

@jtpio
Copy link
Contributor

jtpio commented Oct 27, 2025

Why these checks were failing now 🤷‍♂️

This seems to be the TypeScript build issue according to CI:

src/diff/unified-cell.ts(46,48): error TS2339: Property 'sharedModel' does not exist on type 'Cell<ICellModel>'.

@nakul-py
Copy link
Contributor Author

nakul-py commented Oct 27, 2025

This seems to be the TypeScript build issue according to CI:

Thanks @jtpio the error is fixed now. Idk why this error not comes in when i was building it with jlpm watch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bulk actions (accept / reject) for entire notebooks

3 participants