-
Notifications
You must be signed in to change notification settings - Fork 868
Fix errors when a grouped table is shown #4672
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
base: master
Are you sure you want to change the base?
Conversation
…oups - fix tableholder not scrolling to bottom if there are groups - fix tableholder jumping to top every time a group is collapsed/expanded refs olifolkerd#4219 refs olifolkerd#4525
- fix tableholder jumping to top every time a group is collapsed/expanded refs olifolkerd#4219 refs olifolkerd#4525
|
I have tried these fixes and I no longer have scroll problems on my closed grouped table. Thanks @janhaertel and looking forward to seeing this merged! |
|
This pull request fixes many of my scrolling issues with the vertical virtual renderer and row groups, but not all. Scrolling down with these code changes now works smoothly: if you hold down the thumb of the scrollbar and move it down, it follows the mouse pointer and maintains its vertical distance from the mouse pointer. The issue seems to arise from the line... Adding an element to the top changes the position of the vertical scrollbar, and this triggers a scroll event with an increased scroll position value compared to the last scroll action. The event listener attached in initialize() in RowManager.js recognizes this as a “scrolldown” action (even if the user has scrolled up). This leads to a "_removeTopRow" and "_addBottomRow" call in VirtualDomVertical.js. |
|
This is great! I'm trying to understand the whole situation carefully as well as looking into the issue @Kallius74 mentioned. Hopefully we can merge this soon! |
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 submitting the PR - awesome work! Just one small change request.
|
|
||
| // handle resized columns | ||
| if(this.table.modExists("groupRows")) { | ||
| this.table.modules.groupRows.virtualRenderFill(); |
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.
Could you do this inside the GroupRows module? Just making sure that we are isolating group rows related codes within its own module.
You can probably subscribe to a table-layout event from the GroupRows module and see if that works. If not, please let me know, and we can figure something out.
|
@janhaertel Just checking in to see if you can make the change Azmi requested above? If so, we can merge once the change is made |
If a table has groups which are all collapsed and the VirtualDomVertical renderer is used, there are some issues scrolling the table:
Example
https://jsfiddle.net/isaraystanz/tpgh5vze/11/
I noticed that RowManager.renderTable() will call VirtualDomVertical.renderRows() => _virtualRenderFill() which triggers GroupRows.virtualRenderFill().
In this function the minWidth is adjusted if there are only group rows causing a horizontal scrollbar to be displayed.
Later in RowManager.renderTable(), the call to RowManager.layoutRefresh() will lead to filtColumns.scaleColumns() to be invoked, which changes the width of the columns.
After this, the columns are thin enough to fit the view but the horizontal scrollbar is still being shown, due to the minWidth that was set.
I added a call to GroupRows.virtualRenderFill() after this, so the minWidth will be adjusted and the horizontal scrollbar disappears.
RowManager._virtualRenderFill() will call row.calcHeight() after rows have been rendered causing Group.outerHeight to be set internally. This is always 0 because the implementation for Group.calcHeight() is empty.
The rowsHeight calculated later will then also be 0 since all groups are collapsed.
When the view is scrolled so far, that the next rows have to be added at the bottom and some have to be removed from the top,
VirtualDomVertical._removeTopRow() tries to sum up row heights until none have to be removed anymore.
Since the height is always 0, all rows are added to a list of removable rows then all are removed.
I added a minimal implementation to Group.calcHeight(), so height will not be 0.
Collapsing/Expanding a group will lead to_virtualRenderFill(), (GroupRows.updateGroupRows () ... => RowManager.reRenderInPosition() ...=> _virtualRenderFill()) which sets scrollTop after rendering. Here the position is not correct due to the groupRow height being empty.
This is fixed by the code from the previous issue.
Is the implementation for GroupRow.calcHeight() empty on purpose?
If so the height may have to be calculated differently.
These problems are also addressed in the github issues #4219 and #4525.
fixes #4219
fixes #4525