-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat: Partitioned Table UI Enhancements #2110
feat: Partitioned Table UI Enhancements #2110
Conversation
Looking into unit test failures |
packages/iris-grid/src/mousehandlers/IrisGridPartitionedTableMouseHandler.ts
Outdated
Show resolved
Hide resolved
packages/iris-grid/src/mousehandlers/IrisGridPartitionedTableMouseHandler.ts
Show resolved
Hide resolved
BTW, ready for review |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2110 +/- ##
==========================================
+ Coverage 46.60% 46.68% +0.08%
==========================================
Files 679 691 +12
Lines 38408 38587 +179
Branches 9687 9622 -65
==========================================
+ Hits 17901 18016 +115
- Misses 20455 20560 +105
+ Partials 52 11 -41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Looking into failures here |
Weird that the failures are only occurring on chromium, but I believe the reason of the failure is that we expect to have the pickers accessible in 1.5s, but now, with the new logic, the pickers are disabled while the The reason we do this is so that when we have an empty partition table, we wanted to have the pickers disabled. But even for a table with rows, the table will have 0 rows of length (and thus, will be considered empty) momentarily. We could either change the benchmark to be 20000ms instead of 15000ms, or we could look at adding a specific The latter will be difficult to implement though, since the event listener listens for when the table is updated and then changes the mode accordingly, so I am not sure how before the event, it will differentiate between a table that will be forever empty, and one that is just empty until the data loads in. @mofojed @dsmmcken any thoughts? screen-capture.4.webm |
We've already got the |
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.
I'm going to need a bit more time to think about this one.
packages/iris-grid/src/IrisGrid.tsx
Outdated
@@ -4226,7 +4258,7 @@ class IrisGrid extends Component<IrisGridProps, IrisGridState> { | |||
gotoValueSelectedFilter, | |||
partitionConfig, | |||
} = this.state; | |||
if (!isReady) { | |||
if (!isReady || partitionConfig?.mode === 'loading') { |
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.
I'm going to need to think about this a bit more - we should be showing the loading spinner while it's still loading.
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.
@mofojed, what if we did something like this?
From line 4365-5370 of IrisGrid.tsx
, where we could now set the show
of the loading text based on loadingSpinnerShown
or the configMode === "loading"
No actually, that does not fix the issue we are facing with the spinner, ignore. |
Been doing some more investigations, the firefox and cjromium failures here are not consistent. On one run they fail, but then if I re-run those jobs, they pass. The inconsistent behaviour makes me think there may be a race condition or some weird code circumstance in the code responsible for rendering the context menu. The only change I made to the Context Menu was adding the View Constituent Table option, but I am unsure why that would result in the whole context menu not loading, need to dig more. |
ee26629
to
a092033
Compare
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.
This is looking good, but the e2e tests are failing: https://github.com/deephaven/web-client-ui/actions/runs/9977913349/job/27578476354?pr=2110
I think it's because of our early return in rebuildFilters()
if the filters aren't set, causing it to right-click in the test before the table has appeared.
We can debug in our 1:1, seems kind of tricky
Yeah I have been trying to debug the e2e failures, but it is slightly tricky, will continue to debug and then we can debug together as well in our 1:1, will make the requested change with -1 rn. |
Resolves #2079 ( Resolves #2066 , Resolves #2103 , Resolves #2104 , Resolves #2105 , Resolves #2106 , Resolves #2107 , Resolves #2108 , Resolves #2109 , Resolves #2049 ), Resolves #2120, Resolves #1904
Changes Implemented:
keysTable
withbaseTable
Sample Visuals:
Default Partition Table View
Default Partition-aware source table view: