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

CM6: Fix spacing between editors #728

Merged
merged 10 commits into from
Nov 3, 2023

Conversation

HaudinFlorence
Copy link
Contributor

@HaudinFlorence HaudinFlorence commented Oct 31, 2023

This PR fixes ##720

The gap between the different editors that was put to 10 px has been removed.

CM5 version
Screenshot from 2023-10-31 18-23-51

CM6 version
Screenshot from 2023-10-31 18-27-02

@fcollonval fcollonval added the bug label Nov 1, 2023
@fcollonval
Copy link
Collaborator

Bot please update playwright snapshots

Copy link
Contributor

github-actions bot commented Nov 1, 2023

Playwright ubuntu-22.04 snapshots updated.

Copy link
Contributor

github-actions bot commented Nov 1, 2023

Playwright windows-latest snapshots updated.

@krassowski krassowski closed this Nov 1, 2023
@krassowski krassowski reopened this Nov 1, 2023
@vidartf
Copy link
Collaborator

vidartf commented Nov 1, 2023

In the CM6 screenshot there is still a gap (white) between the local and remote editor and the merged result view. We should get rid of that too before closing #720.

@vidartf
Copy link
Collaborator

vidartf commented Nov 1, 2023

(Happy to merge this as it currently is, as it is a good improvement though!)

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Nov 1, 2023

@vidartf I did not see the white remainig spaces. I am proposing a fix for this in commit c285455 (not sure that it is the best solution) : it consists in adding the cell background color style to the left and right editors (as well as in the central one since I guess it may happen that this white gap occurs in this editor as well). Note that there is still a gap in the gutter.

CM6 version
Screenshot from 2023-11-01 12-23-29

@vidartf
Copy link
Collaborator

vidartf commented Nov 1, 2023

I think the underlying problem is that the central editor is higher for some reason. You can see this even with the old spacing. Inspecting this closer in CSS, it seems to be from some remaining border styles being applied:

/* merge.css */
.jp-Notebook-merge .cm-merge-m-chunk-end-either, .jp-Notebook-merge .cm-merge-m-chunk-end-mixed:not(.cm-merge-m-chunk-custom) {
    border-bottom: 1px solid var(--jp-merge-either-color1);
}

This one should be switched to whatever (non-space taking) style we are using for the other chunk starts/ends.

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Nov 1, 2023

There seems to be a discrepancy between the height of the local and remote editors and the height of the central one. This is due to the fact that adding or not chunk-start and chunk-end decorations makes that lines don't all have the same height. There is a cumulative effect. At some point we proposed a logics adding systematically a transparent border but it has been removed during the review. I don't remember why.
Note there were also some borders surrounding the editors in the CM5 version that are not present in the current CM6 version.

@vidartf
Copy link
Collaborator

vidartf commented Nov 1, 2023

Bot please update playwright snapshots

@HaudinFlorence
Copy link
Contributor Author

I think the underlying problem is that the central editor is higher for some reason. You can see this even with the old spacing. Inspecting this closer in CSS, it seems to be from some remaining border styles being applied:

/* merge.css */
.jp-Notebook-merge .cm-merge-m-chunk-end-either, .jp-Notebook-merge .cm-merge-m-chunk-end-mixed:not(.cm-merge-m-chunk-custom) {
    border-bottom: 1px solid var(--jp-merge-either-color1);
}

This one should be switched to whatever (non-space taking) style we are using for the other chunk starts/ends.

This is normally fixed in commit 90bb3ce
Screenshot from 2023-11-01 13-45-30

@HaudinFlorence
Copy link
Contributor Author

bot please update snapshots

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Nov 1, 2023

There seems to be a discrepancy between the height of the local and remote editors and the height of the central one. This is due to the fact that the adding or not chunk-start and chunk-end decorations makes that lines don't all have the same height. There is a cumulative effect. At some point we proposed a logics adding systematically a transparent border but it has been removed during the review. I don't remember why.

After having a look at the css, I have been updated on the changes. But I don't understand where the style of the decoration for the start / end of the chunks is defined now... without using the border-bottom / bottom-top properties. There are maybe some other leftovers of the old logics remaining (everywhere border properties is used). The cyan color has been lost in commit 90bb3ce for the chunk-start-mixedand chunk-start-enddecorations.

@HaudinFlorence
Copy link
Contributor Author

bot please update snapshots.

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Nov 2, 2023

There seems to be a discrepancy between the height of the local and remote editors and the height of the central one. This is due to the fact that the adding or not chunk-start and chunk-end decorations makes that lines don't all have the same height. There is a cumulative effect. At some point we proposed a logics adding systematically a transparent border but it has been removed during the review. I don't remember why.

After having a look at the css, I have been updated on the changes. But I don't understand where the style of the decoration for the start / end of the chunks is defined now... without using the border-bottom / bottom-top properties. There are maybe some other leftovers of the old logics remaining (everywhere border properties is used). The cyan color has been lost in commit 90bb3ce for the chunk-start-mixedand chunk-start-enddecorations.

I am now ok with this and this should be fixed with c2d47fd. The result is now looking like this for version CM6:
Screenshot from 2023-11-02 09-13-29

@HaudinFlorence
Copy link
Contributor Author

bot please update snapshots

@HaudinFlorence HaudinFlorence reopened this Nov 2, 2023
@HaudinFlorence
Copy link
Contributor Author

bot please update snapshots

@krassowski
Copy link
Member

I think you are missing "playwright". We can change the trigger phase (which we did recently elsewhere) to simplify it, but in this repo it is still:

bot please update playwright snapshots

@HaudinFlorence
Copy link
Contributor Author

I think you are missing "playwright". We can change the trigger phase (which we did recently elsewhere) to simplify it, but in this repo it is still:

bot please update playwright snapshots

My bad... Thanks Mike

Copy link
Contributor

github-actions bot commented Nov 2, 2023

Playwright windows-latest snapshots updated.

Copy link
Contributor

github-actions bot commented Nov 2, 2023

Playwright ubuntu-22.04 snapshots updated.

@krassowski krassowski closed this Nov 2, 2023
@krassowski krassowski reopened this Nov 2, 2023
Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you @HaudinFlorence!

@krassowski krassowski merged commit f5927a1 into jupyter:master Nov 3, 2023
13 checks passed
@krassowski krassowski mentioned this pull request Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants