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

Upgrade to Tabulator 5.3.2 #3784

Merged
merged 8 commits into from
Aug 31, 2022
Merged

Upgrade to Tabulator 5.3.2 #3784

merged 8 commits into from
Aug 31, 2022

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Aug 26, 2022

Fixes #3695

@maximlt
Copy link
Member Author

maximlt commented Aug 26, 2022

@philippjfr I expected this test - test_tabulator_header_filter_no_horizontal_rescroll[None] - to fail. It tests that when you have a wide table with an horizontal scrollbar, then after scrolling right, setting a header filter shouldn't update the scroll position. What happens after the upgrade is that the header filter entirely disappears! I've added a test that checks that this doesn't occur all the time, i.e. it seems to only happen when there's a horizontal scrollbar. Tabulator JS now emits more warnings when events are triggered when they should not, there are a few warnings triggered when the widget is rendered, so maybe that's an area to explore to try to fix that. I've attempted to change some of that logic, without luck unfortunately.

@maximlt
Copy link
Member Author

maximlt commented Aug 28, 2022

Testing this on real apps, I've seen many warnings related to events being triggered when they shouldn't (redraw in particular). I've also seen this error multiple times: Uncaught (in promise) TypeError: Cannot read properties of null (reading offsetWidth).

@philippjfr
Copy link
Member

What happens after the upgrade is that the header filter entirely disappears! I've added a test that checks that this doesn't occur all the time, i.e. it seems to only happen when there's a horizontal scrollbar.

So it doesn't actually disappear, the problem is that the scrollbar disappears when there are no rows, which in turn means you cannot see all the rows in the header. Not sure I'll be able to resolve this immediately.

Tabulator JS now emits more warnings when events are triggered when they should not, there are a few warnings triggered when the widget is rendered, so maybe that's an area to explore to try to fix that. I've attempted to change some of that logic, without luck unfortunately.

Got a fix for this I'll push up shortly.

@philippjfr philippjfr marked this pull request as ready for review August 31, 2022 16:34
@philippjfr
Copy link
Member

Not sure I'll be able to resolve this immediately.

I did resolve it. Had to add this:

  this.tabulator.on("dataFiltered", (_: any, rows: any[]) => {
      if (this._initializing)
	return
      // Ensure that after filtering empty scroll renders
      if (rows.length === 0)
	this.tabulator.rowManager.renderEmptyScroll()
      // Ensure that after filtering the page is updated
      this.updatePage(this.tabulator.getPage())
    })

@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #3784 (3519cdb) into master (7550f75) will increase coverage by 0.03%.
The diff coverage is 98.63%.

@@            Coverage Diff             @@
##           master    #3784      +/-   ##
==========================================
+ Coverage   82.93%   82.96%   +0.03%     
==========================================
  Files         220      221       +1     
  Lines       32047    32205     +158     
==========================================
+ Hits        26579    26720     +141     
- Misses       5468     5485      +17     
Flag Coverage Δ
ui-tests 34.16% <90.41%> (+0.26%) ⬆️
unitexamples-tests 75.42% <35.61%> (-0.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
panel/widgets/tables.py 88.41% <93.33%> (+<0.01%) ⬆️
panel/io/state.py 70.49% <100.00%> (ø)
panel/models/tabulator.py 97.75% <100.00%> (-0.12%) ⬇️
panel/tests/ui/widgets/test_tabulator.py 98.85% <100.00%> (+0.02%) ⬆️
panel/io/reload.py 69.56% <0.00%> (-2.18%) ⬇️
panel/io/server.py 75.19% <0.00%> (-0.69%) ⬇️
panel/io/profile.py 18.95% <0.00%> (-0.13%) ⬇️
panel/io/admin.py 0.00% <0.00%> (ø)
panel/tests/ui/pane/test_plotly.py 97.80% <0.00%> (ø)
panel/widgets/input.py 96.25% <0.00%> (+0.08%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@philippjfr philippjfr merged commit 5f330f2 into master Aug 31, 2022
@philippjfr philippjfr deleted the upgrade_tabulator branch August 31, 2022 16:52
@philippjfr
Copy link
Member

Great work @maximlt!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only part of the Tabulator widget displayed after an update
2 participants