feat: Add row number to data browser #2737
Conversation
|
I will reformat the title to use the proper commit message syntax. |
|
🚀 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. |
Uffizzi Ephemeral Environment
|
Uffizzi Ephemeral Environment
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
📝 Walkthrough""" WalkthroughThe changes introduce a row number column to the data browser interface. A new span element displaying the row number is added to each row in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser as Browser.react.js
participant Table as BrowserTable.react.js
participant Row as BrowserRow.react.js
User->>Browser: Interacts with Data Browser (pagination, scroll)
Browser->>Table: Renders Data Table (passes skip prop)
Table->>Row: Renders each BrowserRow (passes row + skip)
Row->>Row: Displays (row + 1) in sticky row number column
Assessment against linked issues
Suggested labels
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
src/dashboard/Data/Browser/BrowserToolbar.react.js (1)
437-437: Empty div element without clear purposeThis empty
<div/>doesn't seem to serve any functional purpose. Consider removing it or adding a comment explaining its need if it's intended as a placeholder or for layout purposes.- <div/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/BrowserRow/BrowserRow.react.js(1 hunks)src/components/DataBrowserHeaderBar/DataBrowserHeaderBar.scss(1 hunks)src/dashboard/Data/Browser/Browser.react.js(1 hunks)src/dashboard/Data/Browser/Browser.scss(1 hunks)src/dashboard/Data/Browser/BrowserTable.react.js(4 hunks)src/dashboard/Data/Browser/BrowserToolbar.react.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (7)
src/dashboard/Data/Browser/Browser.scss (1)
113-129: Well-implemented sticky row number styling!The new
.rowNumberclass is well-designed with sticky positioning ensuring row numbers remain visible during horizontal scrolling. The style is consistent with existing patterns while adding necessary features for this specific use case.src/components/DataBrowserHeaderBar/DataBrowserHeaderBar.scss (1)
55-57: Appropriate header styling adjustmentThese changes correctly adjust the width and padding to accommodate the new row number column, ensuring proper alignment between header and data rows.
src/components/BrowserRow/BrowserRow.react.js (1)
74-74: Good implementation of row numberingThe row number display is implemented cleanly, with proper 1-indexed conversion for user-friendly display.
Please verify that the
rowprop accounts for pagination offsets to ensure row numbers are continuous across pages.src/dashboard/Data/Browser/Browser.react.js (1)
2106-2106: Pagination offset now properly passed to DataBrowserThis change adds the current pagination offset to the
DataBrowsercomponent, allowing child components to calculate accurate row numbers that account for pagination.src/dashboard/Data/Browser/BrowserTable.react.js (3)
127-127: Row width increased to accommodate row number columnThe additional 30 pixels provides space for the new row number column being added to the data browser interface.
159-159: Row index now accounts for paginationThe row number for edit clone rows now includes the pagination offset (
skip), ensuring consistent row numbering across pages.
332-332: Row index now accounts for paginationThe row number for data rows now includes the pagination offset (
skip), ensuring consistent row numbering across pages.
mtrezza
changed the title
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
Uffizzi Ephemeral Environment
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/dashboard/Data/Browser/BrowserTable.react.js (1)
553-554: Fixed React inline style property.Updated from kebab-case
'overflow-x'to camelCaseoverflowXas required by React's inline style API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/BrowserRow/BrowserRow.react.js(2 hunks)src/components/DataBrowserHeaderBar/DataBrowserHeaderBar.react.js(1 hunks)src/components/DataBrowserHeaderBar/DataBrowserHeaderBar.scss(1 hunks)src/dashboard/Data/Browser/Browser.scss(1 hunks)src/dashboard/Data/Browser/BrowserTable.react.js(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/DataBrowserHeaderBar/DataBrowserHeaderBar.scss
🚧 Files skipped from review as they are similar to previous changes (2)
- src/dashboard/Data/Browser/Browser.scss
- src/components/BrowserRow/BrowserRow.react.js
🧰 Additional context used
🪛 ESLint
src/dashboard/Data/Browser/BrowserTable.react.js
[error] 125-125: 'rowWidth' is never reassigned. Use 'const' instead.
(prefer-const)
🪛 GitHub Check: Lint
src/dashboard/Data/Browser/BrowserTable.react.js
[failure] 125-125:
'rowWidth' is never reassigned. Use 'const' instead
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (7)
src/components/DataBrowserHeaderBar/DataBrowserHeaderBar.react.js (2)
29-30: LGTM: Added headerWidth prop for row number support.The addition of the
headerWidthprop to the component's destructured props will help support the new row numbering feature.
32-32: Properly implemented styling for the checkbox column.The inline style uses the
headerWidthproperty to ensure proper alignment with row numbers by setting appropriate padding and width.src/dashboard/Data/Browser/BrowserTable.react.js (5)
38-38: LGTM: Added headerWidth for row numbering feature.Good initialization of the
headerWidthproperty with a default value of 34 pixels, which will be used for the row number column.
130-134: LGTM: Smart calculation of dynamic header width.The logarithmic calculation for
headerWidthis a clever approach that adapts the width based on the number of digits needed to display the row count, providing better UI as the number of rows increases.
166-170: LGTM: Properly passing headerWidth and updated row calculation.Correct implementation of passing the
headerWidthto BrowserRow components and updating the row index calculation to include pagination offset for consistent row numbering across pages.
341-345: LGTM: Proper row indexing with pagination support.The row index calculation now correctly adds the
skipoffset to ensure continuous row numbering across all pages, which is essential for the row number feature.
576-577: LGTM: Passing headerWidth to DataBrowserHeaderBar.Correctly passing the
headerWidthprop to the DataBrowserHeaderBar component ensures consistent styling between the header and data rows.
| let rowWidth = this.props.order.reduce( | ||
| (rowWidth, { visible, width }) => (visible ? rowWidth + width : rowWidth), | ||
| this.props.onAddRow ? 210 : 0 | ||
| ); | ||
| ) + 30; | ||
|
|
There was a problem hiding this comment.
Use 'const' instead of 'let' for rowWidth.
The rowWidth variable is never reassigned after initialization, so it should use const instead of let to follow best practices and to address the linting error.
- let rowWidth = this.props.order.reduce(
+ const rowWidth = this.props.order.reduce(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let rowWidth = this.props.order.reduce( | |
| (rowWidth, { visible, width }) => (visible ? rowWidth + width : rowWidth), | |
| this.props.onAddRow ? 210 : 0 | |
| ); | |
| ) + 30; | |
| const rowWidth = this.props.order.reduce( | |
| (rowWidth, { visible, width }) => (visible ? rowWidth + width : rowWidth), | |
| this.props.onAddRow ? 210 : 0 | |
| ) + 30; |
🧰 Tools
🪛 ESLint
[error] 125-125: 'rowWidth' is never reassigned. Use 'const' instead.
(prefer-const)
🪛 GitHub Check: Lint
[failure] 125-125:
'rowWidth' is never reassigned. Use 'const' instead
🤖 Prompt for AI Agents (early access)
In src/dashboard/Data/Browser/BrowserTable.react.js around lines 125 to 129, the variable rowWidth is declared with let but never reassigned. Change the declaration from let to const to follow best practices and resolve the linting error.
New Pull Request Checklist
Issue Description
Closes: #2484
Approach
TODOs before merging
Summary by CodeRabbit
New Features
Style
Bug Fixes
Enhancements