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
fix(grid-core): Make a virtual table's viewport calculation band-friendly (T1154239) #3643
Merged
VasilyStrelyaev
merged 7 commits into
DevExpress:master
from
VasilyStrelyaev:band-split-in-fixed-area
Mar 23, 2023
Merged
fix(grid-core): Make a virtual table's viewport calculation band-friendly (T1154239) #3643
VasilyStrelyaev
merged 7 commits into
DevExpress:master
from
VasilyStrelyaev:band-split-in-fixed-area
Mar 23, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
VasilyStrelyaev
changed the title
fix(dx-grid-core): Make virtual table's viewport calculation band-friendly
fix(grid-core): Make virtual table's viewport calculation band-friendly
Mar 22, 2023
VasilyStrelyaev
force-pushed
the
band-split-in-fixed-area
branch
from
March 22, 2023 08:07
a047983
to
c366490
Compare
VasilyStrelyaev
changed the title
fix(grid-core): Make virtual table's viewport calculation band-friendly
fix(grid-core): Make virtual table's viewport calculation band-friendly (T1154239)
Mar 22, 2023
Krijovnick
approved these changes
Mar 22, 2023
MikeVitik
suggested changes
Mar 23, 2023
} | ||
|
||
if (acc.length && acc[acc.length - 1][1] === index - 1) { |
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.
Add constant to describe meaning of: acc.length && acc[acc.length - 1][1] === index - 1
MikeVitik
reviewed
Mar 23, 2023
const isAdjacentToPreviousRange = previousRange && previousRange[1] === index - 1; | ||
|
||
if (isAdjacentToPreviousRange) { | ||
acc.pop(); |
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.
Minor you can replace pop and push to a single splice
MikeVitik
approved these changes
Mar 23, 2023
VasilyStrelyaev
changed the title
fix(grid-core): Make virtual table's viewport calculation band-friendly (T1154239)
fix(grid-core): Make a virtual table's viewport calculation band-friendly (T1154239)
May 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes an issue where header bands that stretch across several fixed columns are split during scrolling: https://supportcenter.devexpress.com/ticket/details/t1154239/reactive-grid-fixed-columns-are-duplicated-if-virtualtable-is-used.
The reason is that when the virtual table calculates the viewport to determine which columns to render, and fixed columns are beyond the regularly calculated viewport, the virtual table adds fixed columns explicitly. However, fixed columns are not merged into a single range but rather added as multiple ranges:
Here is the code that did it: https://github.com/DevExpress/devextreme-reactive/blob/master/packages/dx-grid-core/src/utils/virtual-table.ts#L22
However, bands cannot stretch through multiple viewport ranges, so they get split.
It looks like there is no special reason why adjacent ranges of the viewport cannot be merged together. So, I modified the method that adds fixed columns to the viewport, so that it now merges adjacent indexes into ranges (see my new tests). I aimed not to increase the algorithm's computational complexity as it runs on scroll.
You may notice that the algorithm can now merge both fixed and unfixed columns in a single range. This does not seem to be a problem and it does not prevent a border to appear between fixed and unfixed columns. That's because when splitting the header bands, the grid relies not only on the viewport ranges but also on "column chains", which do break between fixed and unfixed columns.