-
Notifications
You must be signed in to change notification settings - Fork 14
feat: drawer table scroll #2364
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: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR adds automatic scrolling to the selected row in the TopQueries drawer and standardizes row heights by replacing the truncated query component.
- Introduces
useScrollToSelected
hook and wires it intoTopQueriesData
withdynamicInnerRef
- Adds
dynamicRenderType: 'uniform'
to table settings for fixed row heights - Creates
FixedHeightQuery
component (replacingTruncatedQuery
) and corresponding styles
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/containers/Tenant/Diagnostics/TopQueries/utils.ts | Added dynamicRenderType: 'uniform' to table settings |
src/containers/Tenant/Diagnostics/TopQueries/hooks/useScrollToSelected.ts | New hook to scroll the react-list to the selected row |
src/containers/Tenant/Diagnostics/TopQueries/columns/columns.tsx | Swapped out TruncatedQuery for FixedHeightQuery |
src/containers/Tenant/Diagnostics/TopQueries/TopQueriesData.tsx | Integrated scroll hook, updated settings to use dynamicInnerRef |
src/components/FixedHeightQuery/FixedHeightQuery.tsx | New component for rendering queries with fixed height and optional clipboard |
src/components/FixedHeightQuery/FixedHeightQuery.scss | Styles for the fixed-height query display |
Comments suppressed due to low confidence (3)
src/containers/Tenant/Diagnostics/TopQueries/hooks/useScrollToSelected.ts:25
- Consider adding unit tests for this custom hook to verify scrolling logic (visible range check, fallback behavior) under different scenarios.
export function useScrollToSelected({selectedRow, rows, reactListRef}: UseScrollToSelectedParams) {
src/containers/Tenant/Diagnostics/TopQueries/columns/columns.tsx:5
- The
YDBSyntaxHighlighter
import is no longer used after replacingTruncatedQuery
, consider removing this unused import.
import {YDBSyntaxHighlighter} from '../../../../../components/SyntaxHighlighter/YDBSyntaxHighlighter';
src/components/FixedHeightQuery/FixedHeightQuery.tsx:20
- [nitpick] Add a JSDoc comment above the
FixedHeightQuery
component to describe its purpose and the meaning of its props (lines
,hasClipboardButton
, etc.).
export const FixedHeightQuery = ({
src/containers/Tenant/Diagnostics/TopQueries/hooks/useScrollToSelected.ts
Show resolved
Hide resolved
() => ({ | ||
...TOP_QUERIES_TABLE_SETTINGS, | ||
dynamicInnerRef: reactListRef, | ||
// Using 'uniform' type - react-list automatically calculates size from first item |
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.
Redundant comment
@@ -0,0 +1,42 @@ | |||
.kv-fixed-height-query { |
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.
Use .ydb
prefix instead of .kv
please. kv
is Kikimr Viewer - old name of our UI, it is not used anymore, although it's present in some old components
@artemmufazalov fixed issues, added couple of tests |
Closes #2173
Stand
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ✨2 ⏭️1
✨ New Tests (2)
⏭️ Skipped Tests (1)
Bundle Size: 🔺
Current: 83.76 MB | Main: 83.76 MB
Diff: +7.77 KB (0.01%)
ℹ️ CI Information