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

notebook api review comments batch 2 #13012

Merged
merged 13 commits into from
Dec 14, 2023

Conversation

jonah-iden
Copy link
Contributor

@jonah-iden jonah-iden commented Oct 20, 2023

What it does

This is the second batch of Review comments from #12442

this also starts the bigger review task of reorganizing the types from notebook-common

How to test

here you can find a test notebook.

important things to test:

  • kernel selection
  • adding removing cells

Follow-ups

Review checklist

Reminder for reviewers

@jonah-iden jonah-iden force-pushed the jiden/notebook-api-review-comments-batch-2 branch 2 times, most recently from 4d363c0 to c8b9969 Compare October 24, 2023 13:19
@msujew
Copy link
Member

msujew commented Oct 26, 2023

@JonasHelming FYI, I don't really have time to test this appropriately today. Also, I believe the actual improvements to end-users are fairly minor, so it's not all too important to get this into the community release.

@jonah-iden jonah-iden force-pushed the jiden/notebook-api-review-comments-batch-2 branch from b197d77 to 65f52e6 Compare November 1, 2023 07:55
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 didn't look at the behavior yet, but I have a few comments on the code.

better context key reactions of toolbars

Signed-off-by: Jonah Iden <[email protected]>
Signed-off-by: Jonah Iden <[email protected]>
Signed-off-by: Jonah Iden <[email protected]>
Signed-off-by: Jonah Iden <[email protected]>
and type converters

Signed-off-by: Jonah Iden <[email protected]>
Signed-off-by: Jonah Iden <[email protected]>
Signed-off-by: Jonah Iden <[email protected]>
@jonah-iden jonah-iden force-pushed the jiden/notebook-api-review-comments-batch-2 branch from 995c9a4 to 9e4f497 Compare December 14, 2023 09:29
@msujew msujew self-requested a review December 14, 2023 09:53
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.

Thanks, look good to me 👍

@msujew msujew merged commit 0910b02 into master Dec 14, 2023
14 checks passed
@msujew msujew deleted the jiden/notebook-api-review-comments-batch-2 branch December 14, 2023 10:14
@github-actions github-actions bot added this to the 1.45.0 milestone Dec 14, 2023
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.

2 participants