Fix the cell toolbar for Jupytext Notebook factory #1358
Conversation
|
Thank you for making this pull request. Did you know? You can try it on Binder: Also, the version of Jupytext developed in this PR can be installed with (this requires |
|
Thank you @brichet for this PR and fixing that long standing issue! @mahendrapaipuri may I let you review this PR? Thanks! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1358 +/- ##
=======================================
Coverage 96.85% 96.85%
=======================================
Files 32 32
Lines 4924 4924
=======================================
Hits 4769 4769
Misses 155 155
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| for (const extension of docRegistry.widgetExtensions('Notebook')) { | ||
| docRegistry.addWidgetExtension(FACTORY, extension); | ||
| } |
There was a problem hiding this comment.
Is there a race condition here? I mean, what happens if there are widgets extensions that have registered themselves to Notebook factory after Jupytext?
There was a problem hiding this comment.
There is probably a race condition here, this is why we wait for the cell toolbar extension (the one which add the extension to the Notebook factory) to be activated before creating the factory.
There was a problem hiding this comment.
Actually, I missed that there was a signal when a widget extension is added to the registry.
We should probably use it in the factory to keep the factory up to date.
There was a problem hiding this comment.
Yess, that would be way to go if we can make Jupytext subscribe to that signal.
|
Cheers @brichet for the PR. Could you please rebase onto the main and push so that tests will pass. Cheers! |
brichet
force-pushed
the
fix_cell_toolbar
branch
from
8eeca74 to
d67b25f
Compare
brichet
force-pushed
the
fix_cell_toolbar
branch
from
24c0875 to
07afb94
Compare
There was a problem hiding this comment.
Cheers @brichet If CI goes green, I think we are good to go!
| const firstCell = await page.notebook.getCellLocator(0); | ||
| await firstCell?.hover(); | ||
|
|
||
| await expect(firstCell!.locator('.jp-cell-toolbar')).toHaveCount(1); |
There was a problem hiding this comment.
Awesome!! Cheers for the tests, very helpful!
|
Looks good, thanks for the review @mahendrapaipuri |
This PR fixes #990.
The cell toolbar is an extension added to the Notebook factory from the plugin
'@jupyterlab/cell-toolbar-extension:plugin'.With this PR, we make sure that the
cell-toolbar-extensionis activated, and add all the extensions of the Notebook factory to the Jupytext Notebook factory.