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

Simplify rendering logic for grids with merged cells #197

Merged
merged 9 commits into from
Jul 16, 2021
Merged

Simplify rendering logic for grids with merged cells #197

merged 9 commits into from
Jul 16, 2021

Conversation

ibdafna
Copy link
Member

@ibdafna ibdafna commented Jun 22, 2021 •
edited
Loading

This PR:

  • fixes a bug where config.width was wrong for the inorder leftmost child of a merged cell hierarchy.
  • restores optimization for grids with no merged cell regions
  • simplifies rendering logic for grids with merged cell regions

Signed-off-by: Itay Dafna i.b.dafna@gmail.com

EDIT: Will confirm when ready.

@ibdafna
Fix wrong width edge case for column headers
7de6f56 
Signed-off-by: Itay Dafna <i.b.dafna@gmail.com>
@ibdafna ibdafna mentioned this pull request Jun 22, 2021
Merged
@ibdafna ibdafna changed the title Fix wrong width edge case for column headers [WIP] Fix wrong width edge case for column headers Jun 22, 2021
@ibdafna
Use correct column size
a24b6eb 
Signed-off-by: Itay Dafna <i.b.dafna@gmail.com>
@ibdafna ibdafna changed the title [WIP] Fix wrong width edge case for column headers Fix wrong width edge case for column headers Jun 22, 2021
@ibdafna ibdafna changed the title Fix wrong width edge case for column headers Fix wrong width edge case for nested column headers Jun 23, 2021
@ibdafna ibdafna changed the title Fix wrong width edge case for nested column headers [WIP] Fix wrong width edge case for nested column headers Jun 23, 2021
@ibdafna
Copy link
Member Author

ibdafna commented Jun 23, 2021

The code in this PR is actually okay. I am seeing a rendering issue, but it's not caused by this change. I will, however, add the bugfix to this PR to reduce clutter.

@ibdafna
Greatly simplify rendering logic for grid with merged cells
7e67047 
Signed-off-by: Itay Dafna <i.b.dafna@gmail.com>
@ibdafna ibdafna changed the title [WIP] Fix wrong width edge case for nested column headers [WIP] Simplify rendering logic for grids with merged cells Jul 3, 2021
@ibdafna
Fix typo
3caae17 
Signed-off-by: Itay Dafna <i.b.dafna@gmail.com>
@ibdafna
Fix rendering issue with blit logic
706c1b6 
Signed-off-by: Itay Dafna <i.b.dafna@gmail.com>
@ibdafna
Restore _scrollTo() docstring
23e2811 
Signed-off-by: Itay Dafna <i.b.dafna@gmail.com>
@ibdafna ibdafna changed the title [WIP] Simplify rendering logic for grids with merged cells Simplify rendering logic for grids with merged cells Jul 16, 2021
@ibdafna
Add check for data model in _scrollTo
55270f5 
Signed-off-by: Itay Dafna <i.b.dafna@gmail.com>
@ibdafna
Refactor: 'paintEverything' as a private method
46a495c 
Signed-off-by: Itay Dafna <i.b.dafna@gmail.com>
@ibdafna
Refactor 'extendLines' to use same check as 'paintEverything'
f99f5da 
Signed-off-by: Itay Dafna <i.b.dafna@gmail.com>
blink1073
blink1073 approved these changes Jul 16, 2021
View reviewed changes
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks!

@blink1073 blink1073 merged commit 1e757e4 into jupyterlab:master Jul 16, 2021
@fcollonval fcollonval added the bug Something isn't working label Jul 26, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@blink1073 blink1073 blink1073 approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ibdafna @blink1073 @fcollonval