fix: Header row in View table disappears when scrolling up #3105
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
📝 WalkthroughWalkthroughReorganized table/header layout: introduced a fixed-position scrolling container, moved header rendering into the content container, switched header positioning to sticky, adjusted offsets and z-index, and refined header DOM and styles for separate name/type elements and pointer action placement. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/dashboard/TableView.scss (1)
12-23: Consider using CSS custom properties for repeated offset values.The value
96pxappears in multiple places (top: 96pxon line 12,padding-top: 96pxon line 23, and in Views.react.js). While the coordination is correct, using a CSS custom property would make future adjustments easier and reduce the risk of inconsistencies.📋 Example refactor using CSS variables
Define the variable in a shared stylesheet (e.g., globals.scss):
:root { --header-offset: 96px; }Then use it here:
.headers { position: sticky; - top: 96px; + top: var(--header-offset); left: 0; right: 0; background: #66637A; height: 30px; white-space: nowrap; z-index: 10; } .content { position: relative; - padding-top: 96px; + padding-top: var(--header-offset); min-height: 100vh; }src/dashboard/Data/Views/Views.react.js (2)
336-336: Magic number 96 repeated across multiple locations.The value
96pxappears as an inline style in three places (lines 336, 359, 367) and also in TableView.scss. Consider extracting this to a constant or CSS variable for maintainability.📋 Example refactor using a constant
Define a constant at the top of the file:
const HEADER_OFFSET = 96;Then use it:
- <div style={{ paddingTop: 96 }}> + <div style={{ paddingTop: HEADER_OFFSET }}> <div style={{ width: this.state.tableWidth }}> <div className={tableStyles.headers} style={{ width: this.state.tableWidth, right: 'auto', position: 'sticky', - top: 96, + top: HEADER_OFFSET, left: 0, zIndex: 10, }}And for the empty state:
- <div className={tableStyles.empty} style={{ top: 96 }}> + <div className={tableStyles.empty} style={{ top: HEADER_OFFSET }}>Also applies to: 359-359, 367-367
361-374: Inline styles override CSS classes, potentially causing confusion.The headers div uses
className={tableStyles.headers}but then overrides several properties with inline styles (position: 'sticky',top: 96,left: 0,zIndex: 10). This can make it difficult to understand the final computed styles. Consider either:
- Moving these overrides to the SCSS file if they're always needed, or
- Documenting why these inline overrides are necessary for this specific view.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/DataBrowserHeaderBar/DataBrowserHeaderBar.scsssrc/dashboard/Data/Views/Views.react.jssrc/dashboard/TableView.react.jssrc/dashboard/TableView.scss
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Node 18
- GitHub Check: Docker linux/amd64
🔇 Additional comments (3)
src/components/DataBrowserHeaderBar/DataBrowserHeaderBar.scss (1)
15-15: LGTM: Header height now visible.The height change from 0 to 30px makes the sticky header bar visible, which aligns with the PR's goal of fixing header visibility during scroll. This coordinates with the layout adjustments across other files.
src/dashboard/TableView.scss (1)
11-18: Sticky header positioning looks correct.The sticky positioning with
top: 96px,left: 0, andz-index: 10properly keeps the header visible during scroll. The removal of the sidebar offset logic (previouslyleft: 300px) simplifies the layout.src/dashboard/TableView.react.js (1)
41-41: LGTM: Headers correctly moved inside content container.Moving header rendering inside the content container aligns well with the sticky positioning strategy implemented in TableView.scss. The headers are computed earlier (line 41) and rendered within a
styles.headerswrapper (line 59), which coordinates properly with the CSS changes.Note: When data is empty or undefined,
headerswill be null and line 59 will render an empty div. This is acceptable since the empty div won't cause visual issues, but you could optionally conditionally render it:{headers && <div className={styles.headers}>{headers}</div>}Also applies to: 58-61
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/dashboard/Data/Views/Views.react.js (1)
359-384: Consider extracting layout constants for maintainability.The fixed-position scrolling container approach correctly addresses the sticky header requirement. However, the magic numbers
96(toolbar height) and300(sidebar width) are hardcoded in multiple places. Consider extracting these to shared constants or CSS variables to improve maintainability and prevent drift if these dimensions change.💡 Example refactor using constants
+const LAYOUT = { + TOOLBAR_HEIGHT: 96, + SIDEBAR_WIDTH: 300, +}; + renderContent() { // ... code ... return ( <div> <LoaderContainer loading={loading} solid={false}> <div style={{ position: 'fixed', - top: 96, - left: 300, + top: LAYOUT.TOOLBAR_HEIGHT, + left: LAYOUT.SIDEBAR_WIDTH, right: 0, bottom: 0, overflow: 'auto' }}>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/dashboard/Data/Views/Views.react.jssrc/dashboard/Data/Views/Views.scss
🧰 Additional context used
🧬 Code graph analysis (1)
src/dashboard/Data/Views/Views.react.js (1)
src/components/Icon/Icon.react.js (1)
Icon(12-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (6)
src/dashboard/Data/Views/Views.scss (4)
27-37: LGTM!The headerName styling is well-structured with proper text truncation and layout properties. The float-based layout with margin creates appropriate spacing for the split header design.
39-46: LGTM!The headerType styling appropriately creates a visual hierarchy with the smaller font size and muted color, distinguishing it from the primary headerName.
48-87: LGTM!The pointerIcon button styling is well-implemented with proper button resets, absolute positioning that works with the conditional padding in headerWrap, and good accessibility features including focus states and outline.
22-24: Consider browser compatibility for:has()pseudo-class if older browser support is required.The
:has()pseudo-class has limited support: Chrome/Edge 105+, Firefox 121+, Safari 15.4+. Since the project does not document minimum browser version requirements, verify whether older browser support is needed. If it is, implement a fallback using@supportsfeature detection or JavaScript-based conditional padding.src/dashboard/Data/Views/Views.react.js (2)
369-381: LGTM!The sticky header positioning is correctly implemented within the fixed scrolling container. The inline styles appropriately override the CSS class to achieve the sticky behavior, and the background color matches the headerWrap styling in the SCSS file.
606-641: LGTM!The refactored header rendering is well-structured with excellent accessibility:
- Clear separation of name and type information
- Proper button semantics with
type="button"- Descriptive
aria-labelandtitleattributes- Focus management with
blur()after action- Event handling prevents unwanted propagation
The conditional rendering of the pointer button only for Pointer columns is appropriate.
# [8.2.0-alpha.18](8.2.0-alpha.17...8.2.0-alpha.18) (2026-01-07) ### Bug Fixes * Header row in View table disappears when scrolling up ([#3105](#3105)) ([2923e86](2923e86))
|
🎉 This change has been released in version 8.2.0-alpha.18 |
2 participants
New Pull Request Checklist
Summary by CodeRabbit
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.