-
Notifications
You must be signed in to change notification settings - Fork 16
feat(table): introduce recent active markers #3516
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
📝 WalkthroughWalkthroughThe changes introduce a new SCSS mixin for a selected row indicator and a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TableComponent
participant TabulatorRow
User->>TableComponent: Clicks on a row
TableComponent->>TabulatorRow: Activates clicked row
TableComponent->>TableComponent: Push row to activeRowHistory
TableComponent->>TableComponent: If >2, remove oldest from history
TableComponent->>TabulatorRow: Add 'active' class to current row
TableComponent->>TabulatorRow: Add 'recent-active' class to previous row (if any)
TableComponent->>TabulatorRow: Remove 'recent-active' class from other rows
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from the `render` method, they will sometime...
`**/*.{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.
🪛 ESLintsrc/components/table/table.tsx[error] 110-110: No magic number: 2. (no-magic-numbers) ⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (2)
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
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 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
src/components/table/table.tsx
Outdated
this.activeRow = row.getData(); | ||
} | ||
|
||
if (this.activeRowHistory.length > Table.MAX_ACTIVE_ROW_HISTORY) { |
Copilot
AI
Apr 2, 2025
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.
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.
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.
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.tsx
files 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-active
class with its:before
pseudo-element successfully creates a visual indicator for recently active rows. The styling is consistent with the existing.active
class 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-active
class to it. The implementation is clean and follows the existing pattern for theactive
class.
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.
.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
The recent marker is for now only active in selectable rows and not in lists at all, for continuity the recent marker should only be where there is an active marker, which is not in lists at the moment. |
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.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/components/table/partial-styles/tabulator-custom-styles.scss
(2 hunks)src/components/table/table.tsx
(7 hunks)src/style/mixins.scss
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React.
**/*.tsx
: Our.tsx
files 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
🪛 ESLint
src/components/table/table.tsx
[error] 110-110: No magic number: 2.
(no-magic-numbers)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Build
🔇 Additional comments (8)
src/style/mixins.scss (1)
606-620
: Well-structured new mixin for table row indicators.The
table-row-indicator
mixin provides a nice abstraction for the row indicator styling, making it reusable across different table states. The usage of parameters for border color and z-index allows for flexible customization.One small observation - there's a commented-out line referencing a variable that doesn't appear to be defined in this scope:
//z-index: $table--has-interactive-rows--selectable-row--hover + 1;
This comment could be removed or documented better if it serves a specific purpose.
src/components/table/partial-styles/tabulator-custom-styles.scss (2)
48-51
: Good refactoring using the new mixin.Nice improvement - replacing the direct styling with the mixin creates more maintainable code by centralizing the styling logic.
83-88
: Clean implementation of recent-active indicator.The recent-active class is properly implemented using the same mixin as the active row, with a different color to differentiate it visually. This provides good UX by helping users track their interactions with the table.
src/components/table/table.tsx (5)
711-718
: Clean implementation of active row history management.The code for managing the active row history is straightforward and correctly implements the FIFO queue behavior with the size limit.
When a row is clicked:
- It's added to the history
- If the history exceeds the maximum size, the oldest entry is removed
761-768
: Well-structured row class management.This logic correctly applies the 'recent-active' class to the previous active row while ensuring it doesn't conflict with the current active row's styling.
647-647
: Code formatting improvement.Simplified object literal formatting for better readability.
812-812
: Code formatting improvement.Simplified column options object literal for better readability.
831-831
: Code formatting improvement.Simplified Host class binding for better readability.
* Array of rows that have been active | ||
*/ | ||
private activeRowHistory: Tabulator.RowComponent[] = []; | ||
|
||
/** | ||
* Maximum number of rows to keep in the active row history | ||
*/ | ||
private static readonly MAX_ACTIVE_ROW_HISTORY = 2; |
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.
💡 Verification agent
🧩 Analysis chain
Good implementation of active row tracking history.
The active row history tracking implementation is clean and concise. The static constant for the max history size helps maintain code clarity.
Consider using a named constant instead of the magic number 2 to help with code clarity and maintenance:
- private static readonly MAX_ACTIVE_ROW_HISTORY = 2;
+ /**
+ * Maximum number of rows to keep in the active row history
+ * Used to track the current and previous active rows
+ */
+ private static readonly MAX_ACTIVE_ROW_HISTORY = 2;
Let's verify that this constant is the only place where this limit is referenced:
🏁 Script executed:
#!/bin/bash
# Check if the value 2 is hardcoded elsewhere in relation to the active row history
rg -A 1 -B 1 "activeRowHistory" src/components/table/ | grep -E "2|two"
Length of output: 165
Enhance constant documentation for clarity
We’ve confirmed that MAX_ACTIVE_ROW_HISTORY
is the sole reference controlling the “last two” rows, and no other hard-coded 2
appears in the active‐row logic. To improve maintainability, let’s expand its JSDoc to explain why two rows are tracked:
• File: src/components/table/table.tsx
• Location: around line 110
/**
* Maximum number of rows to keep in the active row history
+ * Used to track both the current active row and its immediate predecessor
*/
- private static readonly MAX_ACTIVE_ROW_HISTORY = 2;
+ private static readonly MAX_ACTIVE_ROW_HISTORY = 2;
📝 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.
* Array of rows that have been active | |
*/ | |
private activeRowHistory: Tabulator.RowComponent[] = []; | |
/** | |
* Maximum number of rows to keep in the active row history | |
*/ | |
private static readonly MAX_ACTIVE_ROW_HISTORY = 2; | |
/** | |
* Maximum number of rows to keep in the active row history | |
* Used to track both the current active row and its immediate predecessor | |
*/ | |
private static readonly MAX_ACTIVE_ROW_HISTORY = 2; |
🧰 Tools
🪛 ESLint
[error] 110-110: No magic number: 2.
(no-magic-numbers)
🤖 Prompt for AI Agents (early access)
In src/components/table/table.tsx around lines 103 to 110, enhance the JSDoc
comment for the MAX_ACTIVE_ROW_HISTORY constant by adding an explanation of why
the value 2 is used, clarifying that it represents tracking the last two active
rows for specific functional reasons. This improves code clarity and
maintainability by documenting the rationale behind the chosen limit.
src/style/mixins.scss
Outdated
|
||
@mixin table-row-indicator($border-color, $z-index) { | ||
&: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; | ||
z-index: $z-index; | ||
inset: 0 0 auto 0; | ||
border: $width-of-selected-row-indicator solid $border-color; | ||
border-radius: 1rem; | ||
margin-right: -$width-of-selected-row-indicator * 2; | ||
} | ||
} |
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 great! However you can keep this mixin in the same file where it is used. This is because it is not a reusable mixin for any other component. We only use this file (mixins.scss
) for shared mixins (both in components in this codebase, and in other consumers of Lime Elements). Local mixins are better to be kept where they are used.
margin-right: -$width-of-selected-row-indicator * 2; | ||
} | ||
@include mixins.table-row-indicator( | ||
rgb(0, 150, 136), |
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.
we should never use rgb
or hex
values in our styles. Always use our own color var
s instead.
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3516/ |
src/components/table/partial-styles/tabulator-custom-styles.scss
Outdated
Show resolved
Hide resolved
src/style/mixins.scss
Outdated
} | ||
} | ||
|
||
@mixin table-row-indicator($border-color, $z-index) { |
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.
@mixin table-row-indicator($border-color, $z-index) { | |
@mixin table-row-selected-indicator($border-color, $z-index) { |
Since the mixin is only responsible for visualizing the row that is selected, it's better to name it this way. Otherwise, the reader can be confused about what this piece of code actually does.
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.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/components/table/partial-styles/tabulator-custom-styles.scss
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: SonarCloud
🔇 Additional comments (2)
src/components/table/partial-styles/tabulator-custom-styles.scss (2)
3-4
: Import new color modules for indicator styling
These@use
statements correctly bring in the theme color variables (--mdc-theme-primary
) and the extended palette (--contrast-700
) needed by the new mixin and.recent-active
class. Ensure that no unused imports remain after further styling updates.
64-67
: Apply mixin for active row indicator
Replacing the manual pseudo-element with the mixin here is spot on. Please verify that the computed$table--has-interactive-rows--selectable-row--hover + 1
z-index places the indicator above all row content and doesn’t conflict with other interactive layers.
@mixin table-row-selected-indicator($border-color, $z-index) { | ||
&:before { | ||
$width-of-selected-row-indicator: 0.2rem; | ||
content: ''; | ||
display: inline-block; | ||
box-sizing: border-box; | ||
position: sticky; | ||
z-index: $z-index; | ||
inset: 0 0 auto 0; | ||
border: $width-of-selected-row-indicator solid $border-color; | ||
border-radius: 1rem; | ||
margin-right: -$width-of-selected-row-indicator * 2; | ||
} | ||
} |
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.
🧹 Nitpick (assertive)
DRY refactor: extract row indicator pseudo-element into a mixin
Great consolidation of the repeated :before
pseudo-element styling into @mixin table-row-selected-indicator
. As an optional improvement, you could expose the $width-of-selected-row-indicator
as a mixin parameter to make this more flexible for different indicator sizes.
🤖 Prompt for AI Agents (early access)
In src/components/table/partial-styles/tabulator-custom-styles.scss between
lines 6 and 19, the mixin table-row-selected-indicator currently uses a fixed
width for the selected row indicator. To improve flexibility, add a new
parameter to the mixin for the width of the selected row indicator, replacing
the hardcoded value. Update all references inside the mixin to use this
parameter instead of the fixed 0.2rem value.
.recent-active { | ||
@include table-row-selected-indicator( | ||
rgb(var(--contrast-700)), | ||
$table--has-interactive-rows--selectable-row--hover + 1 | ||
); | ||
} |
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.
🛠️ Refactor suggestion
Scope and z-index of .recent-active
should differ from the active indicator
The new .recent-active
class currently sits at the same specificity and z-index as .active
. To avoid stacking conflicts and unintended styling on non-row elements, consider:
- Nesting the selector under
.tabulator-row.selectable
(e.g..tabulator-row.selectable.recent-active
) - Using a slightly lower z-index (for example,
$table--has-interactive-rows--selectable-row--hover
) so the recent marker appears behind the current one
Example diff:
@@ :host(.has-interactive-rows) {
- .recent-active {
+ .tabulator-row.selectable.recent-active {
@include table-row-selected-indicator(
rgb(var(--contrast-700)),
- $table--has-interactive-rows--selectable-row--hover + 1
+ $table--has-interactive-rows--selectable-row--hover
);
}
🤖 Prompt for AI Agents (early access)
In src/components/table/partial-styles/tabulator-custom-styles.scss around lines
99 to 104, the .recent-active class has the same scope and z-index as the
.active indicator, causing stacking conflicts. To fix this, nest the
.recent-active selector under .tabulator-row.selectable (e.g.,
.tabulator-row.selectable.recent-active) to increase specificity, and adjust its
z-index to be slightly lower, such as using
$table--has-interactive-rows--selectable-row--hover, so the recent-active marker
appears behind the active indicator.
two things I can think of:
|
Is this a blocker to getting this PR in? Should we create issues for those other views? |
I think it's just hard to remember to check and verify it in the #3478 And I think we can make an umbrella issue for all views, and connect this as a sub-task to that one.
|
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: