-
Notifications
You must be signed in to change notification settings - Fork 160
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
Support JupyterLab 4.0, port to Lumino 2 #673
Conversation
5f8c05d
to
0f79987
Compare
Hooray, green! Two suggestions:
|
@krassowski Thanks for your suggestion. I have addressed the first point concerning the PR description. The duplicated commit has been squashed. |
@krassowski Sorry if it is obvious, but where do you think I should document theses changes? |
…), add a listener, try to reintroduce syntax highlighting (not yet working) and introduce minor css changes.
Update onGutterClick method, syncModel Update of the mergeView Perform some renaming Add methods to remove the line-chunk mapping and the gutter markers Refactoring of the code : remove most of create effect methods except for the gutter effects.
Choose the extensions to be added from Jupyterlab default ones Migrate the synchronized scrolling Add transparent borders to the chunk decorations Update mergeview.ts and relative styling to make nbdiff work
… diff is implemented) Update css files
Update mergeview.ts and editor.ts with minor changes Update example9 files. Update mergeview.ts to take review comments into account. Update css files with minor changes. Update to take review into account. Keep on taking review into account regarding the iterator issue, dependencies and CI troubles. Add jest types Try fix CI js failing tests and try to fix PatchObjectHelper iterator. Remove any reference to jupyter_server_mathjax and related references. Fix iterator implementation on `PatchObjectHelper` Fix tests Fix Python test failing due to retained decorator Remove one more remnant of old mathjax Disable auto-detection of language servers to reduce startup time Add ui-tests to github workflows. Add missing directory with example notebooks data/default/test2. Try to make ui-tests work. Update ./github/workflows/tests.yml Update .github/workflows/tests.yml Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com> Remove last commit change adding option --update-snapshots for npx playwright test and add update-integration-tests.yml for to .github/workflows. Update .github/workflows/tests.yml Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com> Update .github/workflows/update-integration-tests.yml Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com> Take comments into account regarding test.yml file and tests. Apply suggestions from code review Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com> Remove `set -eux` because it is Unix-only Use npm client Actually pass python version to setup script Add missing `run` Add comment/reaction steps Add polyfill for playwright npx run Only update snapshots on latest Python, run one at a time to avoid git conflicts Update Playwright Snapshots Apply suggestions from code review Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com> Restore green colors backgrounds for the right editor for a diff. Fix the added cells CSS style. Fix alignment shift for diff in alignViews. Fix `lineChunks` and editor configuration, including readOnly Remove hard-coded `lineNumbers` option Update mergeview.ts and diff.css to try to fix the added or deleted cells backgrounds. Take comment into account to fix the background of added/deleted cells. Restore `getMergedValue()` Restore border indicator, remove unused .CodeMirror selector Add ui-tests for diff and change ui-tests names and descriptions. Fix dep specification Restore CSS scoping Reduce diff due to format changes Restore `getMergedValue` Improve integration tests Improve integration tests Avoid duplicating example/test contents Instantiate registries only once Only create editor factory (and associated registries) once Don't remove favicon when cleaning Remove old typings Fix style not deduplicated Use only prebuilt extension Fix jobs config Fix jest tests Fix integration tests Fix wheel name for windows Pass wheel name to ui-test job Update symlink Update Playwright Snapshots Update Playwright Snapshots Update ui-tests/tests/nbdime-merge-test1.spec.ts
0f79987
to
711fa95
Compare
I would just add inline comments for cases where the internet may not be obvious. |
I have added a section in the description of the PR called "Additionnal comments on changes" and a commit will be added with this comments added in the code too. Please feel free to mention what important points should be added from your point of view @krassowski @fcollonval @JohanMabille. Thanks. |
cdcd7a4
to
ecc4478
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot to all of you and especially @HaudinFlorence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, it will be super helpful to have nbdime on JupyterLab 4!
During the lab call @vidartf asked about dark theme. I can confirm that for JupyterLab diff mode works in dark mode.
It would be good to restore the background of cells in dark mode, which in my opinion could be in a follow-up PR. The comments on changed lines are hard to read bu this is not a regression. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial diff scan of the files that do not include editor/mergeview. Doing this first, as it should be quicker to discuss questions for these.
examples/example1
Outdated
@@ -0,0 +1 @@ | |||
../ui-tests/data/merge_test1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the files in this examples folder meant to be used for? How does it differ from the files in the ui-tests folder, and the nbdime/tests/files folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could reduce the number of examples in the dedicated folders.They were used during the migration process to check various diff and merge configurations.
The diff in ui-tests/diff_test1
and ui-tests/diff_test2
are variants of example1 (center.ipynb with addition/deletions of additionnal cells) and ui-tests/diff_test3
is a variant of example8
(with left.ipynb
and center.ipynb
notebooks).
The merge in ui-tests/merge_test1
and ui-tests/merge_test2
are variant of example1 and ui-tests/merge_test3
is a variant of example3
.
nbdime/tests/files folder
has nothing in common with ui-tests
and examples
.
Co-authored-by: Vidar Tonaas Fauske <vidartf@gmail.com>
Re the OP's "Styling/UX differences" sections. Is that up to date? Would you mind clarifying which of these points are changed because of technical challenges, and which (if any) are considered improvements? E.g. why was the ability to have collapsed sections removed, and what would the challenges be to add it back in? |
@vidartf many thanks for your review. The different comments should be addressed the commit labelled as 'Take review comments into account' 63c9c24. Concerning the styling/UX differences section, it is up to date and I have clarified these differences by classified them between two sections : the first one is entitled "features to be restored" and the second one is named "chosen differences or differences related to CM6". Feel free to comment if you think that some missing features have to be restored (e.g. the smalls arrows in the diff application ?) or if some may be given up (like coloring the spacers in the merge application ?). |
@@ -18,8 +22,13 @@ const ROOT_METADATA_CLASS = 'jp-Metadata-diff'; | |||
* MetadataWidget for changes to Notebook-level metadata | |||
*/ | |||
export class MetadataDiffWidget extends Panel { | |||
constructor(model: IStringDiffModel) { | |||
// TODO improve typing hierarchy to avoid `Omit` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this TODO for yourself, or for someone in the future? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this comment is mine so I don't know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it - I will remove it when working into the integration with @jupyterlab/git
@vidartf are you concerns addressed? Could we merge this PR? Could you also accept the NPM invitation to join the jupyter organization? Once accepted I will have to increase your rights for you to add the nbdime package as it is not possible to grant new member advanced rights by default. |
@fcollonval My previous first-pass concerns are indeed addressed, and I've convinced myself that the styling concerns I have can be iterated on in follow-up PRs (I have fixes for most of them locally). My current major concern is that the mergetool seems to be extremely unreliable (clicking the pickers sometimes work, and sometimes not at all), and I am trying to figure out exactly what is causing this. I'm going to spend some time today to understand it, and how much work a fix would entail, then I'll get back to you. |
Never mind the mergetool. The examples I found that broke it seems to be broken in previous releases of nbdime as well. I will create an issue for that separately. |
(actually, the mergetool is more generally broken in this PR than previous releases, but I think we can merge and iterate on that...) |
Congrats to @HaudinFlorence, this is a significant achievement! Many thanks to everyone involved (@vidartf @krassowski @fcollonval @JohanMabille). |
Yes, thanks again @HaudinFlorence . |
@HaudinFlorence I'm not sure if I fully understood what this meant. Can you help clarify?
|
@vidartf It seemed to me that in the case of the merge app (mostly when there are more than one cells), the blue indication after the cell mentioning "Metadada changed" was missing. I am adding a capture of how it looks in CM5 version (not able to build CM6 version right now.) |
This PR is a rebase of #664. It includes fixes following the rebase with the PR adding the linter.
An updated description of the PR is reproduced just below
PR #664 is a first pass on trying a migration to jupyterlab4. It includes :
Main changes
This PR includes among others the migration to CM6 of these features of the web applications:
Diff app rendering example
Merge app rendering example
Styling/UX differences
features to be restored
chosen differences or differences related to CM6
sinus
function in the central editor in the merge example above). (later erratum : the approach is mixed with both syntax highlighting and diff coloring, one of the 2 approaches should be chosen.)Additionnal comments on changes
Future work and follow up tasks
In some cases, the result of the native function
findAlignedLines
is not right leading to a misalignment of lines to be put at the same height and to incorrect treatment of adding paddings. An issue has been opened with an example findAlignedLines sometimes returns unexpected results #669More ui tests will be added when
findAlignedLines
will be fixed for the non working caseIn reference to comment Upgrade nbdime for jupyterlab 4 #664 (comment), changes will have to be made after the PR is merged regarding use of
find
,each
,toArray
related to Lumino to move to JS equivalent.