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.

Sign up for GitHub

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

Jump to bottom

Deal with chunk to align outside of viewport #694

Closed
wants to merge 1 commit into from
Closed

Deal with chunk to align outside of viewport #694

wants to merge 1 commit into from

Conversation

fcollonval
Copy link
Collaborator

@fcollonval fcollonval commented Sep 25, 2023 •
edited
Loading

Fixes #693

If some alignments are outside of the viewport, this adds an initial spacers to have persist the alignments.

  • Fix test to remove collapsing identical lines...

@fcollonval
Copy link
Collaborator Author

bot please update playwright snapshots

@github-actions
Copy link
Contributor

Playwright ubuntu-22.04 snapshots updated.

@github-actions
Copy link
Contributor

Playwright windows-latest snapshots updated.

@fcollonval
Copy link
Collaborator Author

Kicking the CI

@fcollonval fcollonval closed this Sep 26, 2023
@fcollonval fcollonval reopened this Sep 26, 2023
@fcollonval fcollonval added the bug label Sep 28, 2023
krassowski
krassowski reviewed Oct 2, 2023
View reviewed changes
...sts/nbdime-diff-test4.spec.ts-snapshots/diff-test4-The-file-ends-must-be-aligned-1-win32.png
Copy link
Member

Choose a reason for hiding this comment

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

This looks quite different from the linux snapshot is this expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks Mike indeed it should be aligned too; gonna need a Win machine.

@fcollonval fcollonval force-pushed the fix/693-Vertical-spacer-outside-of-viewport-are-not-included branch from e457924 to 5aeb9e0 Compare October 10, 2023 08:25
@fcollonval
Copy link
Collaborator Author

bot please update playwright snapshots

@github-actions
Copy link
Contributor

Playwright windows-latest snapshots updated.

@github-actions
Copy link
Contributor

Playwright ubuntu-22.04 snapshots updated.

@fcollonval fcollonval closed this Oct 10, 2023
@fcollonval fcollonval reopened this Oct 10, 2023
@fcollonval fcollonval force-pushed the fix/693-Vertical-spacer-outside-of-viewport-are-not-included branch 2 times, most recently from a1baf95 to edcd71d Compare October 10, 2023 12:27
@fcollonval
Copy link
Collaborator Author

bot please update playwright snapshots

@github-actions
Copy link
Contributor

Playwright ubuntu-22.04 snapshots updated.

@github-actions
Copy link
Contributor

Playwright windows-latest snapshots updated.

@fcollonval fcollonval mentioned this pull request Oct 11, 2023
Merged
@vidartf
Copy link
Collaborator

vidartf commented Nov 6, 2023

Is this still relevant? If you rebase (and maybe explain the issue a bit more), I can have a look on Windows.

@fcollonval
Fix spacer remove when out of viewport
eae4c1f 
Add test proving it is broken

Add some delay before scrolling to the bottom

Make the test more obvious
@fcollonval fcollonval force-pushed the fix/693-Vertical-spacer-outside-of-viewport-are-not-included branch from c8d1e86 to eae4c1f Compare November 7, 2023 13:02
@fcollonval
Copy link
Collaborator Author

I'm closing this one as after rebase - the test is working even without the code change.

FYI the issue I saw was:
if the editor is not rendering all lines (CM optimization) and if in the not-rendered lines above the current viewport there should be some vertical spacers to align the code, those spacers were not added (the anchor line being out of the viewport). That led to misalignment of the code in the viewport.

@fcollonval fcollonval closed this Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krassowski krassowski krassowski left review comments

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Vertical spacer outside of viewport are not included
3 participants
@fcollonval @vidartf @krassowski