feat(table): introduce recent active markers #3516
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a visual enhancement and an active row tracking feature. A new CSS class Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant T as Table Component
participant S as Styling Engine
U->>T: Click on a table row
T->>T: Check activeRowHistory length
alt activeRowHistory exceeds MAX
T->>T: Remove oldest row entry
end
T->>T: Add clicked row to activeRowHistory
T->>T: Call formatRow for rendering
alt Row is second most recently activated
T->>S: Apply .recent-active style
end
✨ Finishing Touches
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. 🪧 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
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces recent active row markers to the table component by tracking recently activated rows.
- Added an activeRowHistory array and a MAX_ACTIVE_ROW_HISTORY constant to limit its size
- Updated row activation logic to store active rows and conditionally add a visual "recent-active" marker
- Minor formatting improvements for conciseness
Files not reviewed (1)
- src/components/table/partial-styles/tabulator-custom-styles.scss: Language not supported
| @@ -705,7 +708,17 @@ export class Table { | |||
| this.activeRow = row.getData(); | |||
| } | |||
|
|
|||
| if (this.activeRowHistory.length > Table.MAX_ACTIVE_ROW_HISTORY) { | |||
There was a problem hiding this comment.
The clicked row is only being added to the history when the length is already greater than MAX_ACTIVE_ROW_HISTORY, which may prevent initial active rows from getting recorded. Consider always pushing the clicked row to activeRowHistory and then trimming the array if its length exceeds MAX_ACTIVE_ROW_HISTORY.
| if (this.activeRowHistory.length > Table.MAX_ACTIVE_ROW_HISTORY) { |
Copilot uses AI. Check for mistakes.
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/components/table/partial-styles/tabulator-custom-styles.scss(1 hunks)src/components/table/table.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React.
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React.
src/components/table/table.tsx
`**/*.{ts,tsx}`: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
src/components/table/table.tsx
🪛 GitHub Actions: Pull Request Checks
src/components/table/table.tsx
[error] 724-724: TypeScript: Declaration or statement expected.
🔇 Additional comments (5)
src/components/table/partial-styles/tabulator-custom-styles.scss (1)
90-105: Good implementation of visual indicator for recently active rows.The new
.recent-activeclass with its:beforepseudo-element successfully creates a visual indicator for recently active rows. The styling is consistent with the existing.activeclass pattern but uses a lighter color (rgb(211, 211, 211)) to differentiate it from the currently active row.src/components/table/table.tsx (4)
103-111: Good addition of active row history tracking.The new private properties for tracking active row history are well-named and documented. The use of a static constant for the maximum history size makes the code more maintainable.
762-771: Good implementation of recent active row highlighting.The code correctly identifies the previous active row from the history and applies the
recent-activeclass to it. The implementation is clean and follows the existing pattern for theactiveclass.
647-647: Approved code style improvement.Condensing the load object into a single line improves code readability.
815-815: Approved code style improvement.Condensing the column options object into a single line is a good refactoring for consistency.
| if (this.activeRowHistory.length > Table.MAX_ACTIVE_ROW_HISTORY) { | ||
| const clickedRow = this.tabulator.getRow(row.getData().id); | ||
| this.activeRowHistory.push(clickedRow); | ||
|
|
||
| // Keep the history array limited to the last two rows | ||
| if (this.activeRowHistory.length > 2) { | ||
| this.activeRowHistory.shift(); | ||
| } | ||
|
|
||
| this.activate.emit(this.activeRow); | ||
| this.formatRows(); |
There was a problem hiding this comment.
Fix the syntax error in the active row history logic.
There's a syntax error in the conditional logic for managing the activeRowHistory. The condition on line 711 is checking if the length exceeds MAX_ACTIVE_ROW_HISTORY, but there's no corresponding closing brace for this condition before continuing execution.
Apply this diff to fix the issue:
- if (this.activeRowHistory.length > Table.MAX_ACTIVE_ROW_HISTORY) {
const clickedRow = this.tabulator.getRow(row.getData().id);
this.activeRowHistory.push(clickedRow);
// Keep the history array limited to the last two rows
if (this.activeRowHistory.length > 2) {
this.activeRowHistory.shift();
}Either remove the first condition or properly structure the conditional blocks:
const clickedRow = this.tabulator.getRow(row.getData().id);
this.activeRowHistory.push(clickedRow);
// Keep the history array limited to the last two rows
if (this.activeRowHistory.length > Table.MAX_ACTIVE_ROW_HISTORY) {
this.activeRowHistory.shift();
}📝 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.
| if (this.activeRowHistory.length > Table.MAX_ACTIVE_ROW_HISTORY) { | |
| const clickedRow = this.tabulator.getRow(row.getData().id); | |
| this.activeRowHistory.push(clickedRow); | |
| // Keep the history array limited to the last two rows | |
| if (this.activeRowHistory.length > 2) { | |
| this.activeRowHistory.shift(); | |
| } | |
| this.activate.emit(this.activeRow); | |
| this.formatRows(); | |
| const clickedRow = this.tabulator.getRow(row.getData().id); | |
| this.activeRowHistory.push(clickedRow); | |
| // Keep the history array limited to the maximum number of active rows | |
| if (this.activeRowHistory.length > Table.MAX_ACTIVE_ROW_HISTORY) { | |
| this.activeRowHistory.shift(); | |
| } | |
| this.activate.emit(this.activeRow); | |
| this.formatRows(); |
| 'has-low-density': this.layout === 'lowDensity', | ||
| }} | ||
| > | ||
| <Host class={{ 'has-low-density': this.layout === 'lowDensity' }}> |
There was a problem hiding this comment.
There was a problem hiding this comment.
What do you mean @Kiarokh?
There was a problem hiding this comment.
I meat to link it to the previous comment about auto linting :)
| .recent-active { | ||
| &:before { | ||
| $width-of-selected-row-indicator: 0.2rem; | ||
| content: ''; | ||
| display: inline-block; | ||
| box-sizing: border-box; | ||
| position: sticky; | ||
| z-index: $table--has-interactive-rows--selectable-row--hover + | ||
| 1; | ||
| inset: 0 0 auto 0; | ||
| border: $width-of-selected-row-indicator solid | ||
| rgb(211, 211, 211); | ||
| border-radius: 1rem; | ||
| margin-right: -$width-of-selected-row-indicator * 2; | ||
| } | ||
| } |
There was a problem hiding this comment.
this is mostly repetition of lines 48 to 60.
it's better to either make a mixin for these lines; or have a common CSS rule for both. The only difference is the colors of the dots, which can the be written in a separate CSS rule for each
fix: https://github.com/Lundalogik/crm-feature/issues/1294
Summary by CodeRabbit
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: