chore(table): update tabulator to 6 #3478
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughUpgrades tabulator-tables from v4 to v6, refactoring type imports to explicit types instead of namespace imports across multiple files. Updates table initialization logic with a new createTabulator factory and event listener pattern. Adds paginationLocation prop to control pagination UI placement. Updates SCSS styling with CSS variables and selector renames. Removes custom type declaration file. Changes
Sequence DiagramsequenceDiagram
participant Component as Table Component
participant Factory as createTabulator Factory
participant Tabulator as Tabulator Instance
participant Listeners as Event Listeners
Component->>Factory: Call createTabulator(table)
Factory->>Tabulator: Instantiate Tabulator
Factory->>Listeners: Attach rowClick listener
Factory->>Listeners: Attach dataSorting listener
Factory->>Listeners: Attach pageLoaded listener
Factory->>Listeners: Attach columnMoved listener
Factory->>Listeners: Attach renderComplete listener
Factory->>Listeners: Attach tableBuilt listener
Tabulator->>Listeners: Emit tableBuilt event
Listeners->>Component: Set initialized = true
alt Remote Mode
Component->>Tabulator: setData with AJAX (paginated)
else Local Mode
Component->>Tabulator: setData with local data
end
Tabulator->>Component: Render table with pagination
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3478/ |
|
Not sure if anything from this old PR is useful here. But might be worth to double check #1712 |
jgroth
force-pushed
the
upgrade-tabulator
branch
from
537e3ab to
8803277
Compare
I checked the old PR and made some updates. All examples seems to work now |
I had a quick look. It seems the only thing that is different is our customization for the sorting icon in column headers which is gone now. But maybe we can live with that (if the directions are as we intended them to be, as described in here: https://github.com/Lundalogik/crm-feature/issues/4413) I need to check |
This comment was marked as resolved.
This comment was marked as resolved.
Kiarokh
force-pushed
the
upgrade-tabulator
branch
from
f78bd9c to
8b63269
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/components/table/table.scss (1)
72-78: Hover rule will never match – wrong element combination
.tabulator-sortable.tabulator-col-sorter-elementrequires both classes on the same node.
In Tabulator 6 the sorter element is inside.tabulator-sortable, not the same element.
Result: header hover background is lost.Apply either of the following fixes:
-&.tabulator-sortable.tabulator-col-sorter-element { - &:hover { - background-color: rgb(var(--table-header-background-color--hover)); - } -} +.tabulator-sortable:hover { + background-color: rgb(var(--table-header-background-color--hover)); +}or, if you really want to scope to the sorter element:
-.tabulator-sortable.tabulator-col-sorter-element:hover +.tabulator-sortable .tabulator-col-sorter-element:hover
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.mdpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
package.json(2 hunks)src/components/table/columns.ts(9 hunks)src/components/table/examples/table-pagination.tsx(1 hunks)src/components/table/partial-styles/_row-selection.scss(1 hunks)src/components/table/partial-styles/_table-main-layout.scss(1 hunks)src/components/table/partial-styles/tabulator-arrow.scss(4 hunks)src/components/table/partial-styles/tabulator-custom-styles.scss(2 hunks)src/components/table/table-selection.spec.ts(3 hunks)src/components/table/table-selection.ts(6 hunks)src/components/table/table.scss(3 hunks)src/components/table/table.tsx(20 hunks)types/tabulator-tables/index.d.ts(0 hunks)
💤 Files with no reviewable changes (1)
- types/tabulator-tables/index.d.ts
🧰 Additional context used
📓 Path-based instructions (6)
`src/components/**/*.scss`: Do not use BEM-style class names in component CSS files, as components use shadow-DOM.
src/components/**/*.scss: Do not use BEM-style class names in component CSS files, as components use shadow-DOM.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/css-class-names-in-components-using-shadow-dom.mdc)
List of files the instruction was applied to:
src/components/table/partial-styles/_row-selection.scsssrc/components/table/partial-styles/_table-main-layout.scsssrc/components/table/partial-styles/tabulator-custom-styles.scsssrc/components/table/table.scsssrc/components/table/partial-styles/tabulator-arrow.scss
`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/components/table/partial-styles/_row-selection.scsssrc/components/table/partial-styles/_table-main-layout.scsssrc/components/table/partial-styles/tabulator-custom-styles.scsssrc/components/table/table.scsssrc/components/table/examples/table-pagination.tsxsrc/components/table/partial-styles/tabulator-arrow.scsssrc/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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/components/table/table-selection.spec.tssrc/components/table/table-selection.tssrc/components/table/examples/table-pagination.tsxsrc/components/table/columns.tssrc/components/table/table.tsx
`**/*.{tsx,jsx}`: When returning multiple JSX elements from the `render` method,...
**/*.{tsx,jsx}: When returning multiple JSX elements from therendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element. When there is already a single top-level element, it does not have to be wrapped in<Host>.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/wrap-multiple-jsx-elements-in-host-not-in-array.mdc)
List of files the instruction was applied to:
src/components/table/examples/table-pagination.tsxsrc/components/table/table.tsx
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a de...
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/components/table/examples/table-pagination.tsxsrc/components/table/table.tsx
`src/components/**/examples/**/*.{ts,tsx}`: These files are an exception to the ...
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publically exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/components/table/examples/table-pagination.tsx
🧠 Learnings (10)
src/components/table/partial-styles/_row-selection.scss (1)
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3518
File: src/components/chip-set/chip-set.tsx:367-385
Timestamp: 2025-04-14T12:22:22.298Z
Learning: In limel-chip-set, the onClick handler should be placed on the content div (<div slot="content">) rather than on the limel-notched-outline component, and should only be applied when type is 'input'.
src/components/table/partial-styles/_table-main-layout.scss (1)
Learnt from: CR
PR: Lundalogik/lime-elements#0
File: .cursor/rules/css-class-names-in-components-using-shadow-dom.mdc:0-0
Timestamp: 2025-07-01T14:43:04.238Z
Learning: Applies to src/components/**/*.scss : Do not use BEM-style class names in component CSS files, as components use shadow-DOM.
src/components/table/partial-styles/tabulator-custom-styles.scss (2)
Learnt from: CR
PR: Lundalogik/lime-elements#0
File: .cursor/rules/css-class-names-in-components-using-shadow-dom.mdc:0-0
Timestamp: 2025-07-01T14:43:04.238Z
Learning: Applies to src/components/**/*.scss : Do not use BEM-style class names in component CSS files, as components use shadow-DOM.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3539
File: src/components/text-editor/text-editor.scss:14-14
Timestamp: 2025-04-24T10:16:05.891Z
Learning: In SCSS, prefer using `calc()` function over native SCSS arithmetic operations (like `$variable / 2`) as it aligns with modern SASS recommendations for better CSS compatibility and interoperability with CSS variables.
src/components/table/table.scss (1)
Learnt from: CR
PR: Lundalogik/lime-elements#0
File: .cursor/rules/css-class-names-in-components-using-shadow-dom.mdc:0-0
Timestamp: 2025-07-01T14:43:04.238Z
Learning: Applies to src/components/**/*.scss : Do not use BEM-style class names in component CSS files, as components use shadow-DOM.
src/components/table/table-selection.spec.ts (2)
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using '@limetech/lime-elements' rather than defining duplicate types locally. This includes types like `EditorUiType` which are part of the component's public API.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-16T14:14:18.253Z
Learning: For lime-elements, example files should be self-contained and avoid importing internal implementation details. This includes not importing types from internal files like '../types.ts', especially those marked with @beta. Duplicating simple type definitions in example files is preferred over importing internal types that may change.
src/components/table/table-selection.ts (1)
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using '@limetech/lime-elements' rather than defining duplicate types locally. This includes types like `EditorUiType` which are part of the component's public API.
src/components/table/examples/table-pagination.tsx (4)
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-16T14:14:18.253Z
Learning: For lime-elements, example files should be self-contained and avoid importing internal implementation details. This includes not importing types from internal files like '../types.ts', especially those marked with @beta. Duplicating simple type definitions in example files is preferred over importing internal types that may change.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-16T14:14:18.253Z
Learning: For lime-elements, example files should be self-contained and avoid importing internal implementation details. Example files should only import public exports from '@limetech/lime-elements' or use relative imports for other files within the examples folder. Duplicating simple type definitions in example files is preferred over importing internal types.
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:1-11
Timestamp: 2025-03-04T14:27:29.714Z
Learning: In example components (typically found in `src/components/*/examples/`), imports of lime-elements exports should use `@limetech/lime-elements` instead of relative paths. This is because example components aren't included in type declarations and are used in documentation to show developers how to properly use lime-elements components.
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using '@limetech/lime-elements' rather than defining duplicate types locally. This includes types like `EditorUiType` which are part of the component's public API.
src/components/table/partial-styles/tabulator-arrow.scss (2)
Learnt from: CR
PR: Lundalogik/lime-elements#0
File: .cursor/rules/css-class-names-in-components-using-shadow-dom.mdc:0-0
Timestamp: 2025-07-01T14:43:04.238Z
Learning: Applies to src/components/**/*.scss : Do not use BEM-style class names in component CSS files, as components use shadow-DOM.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3529
File: src/components/lime-ai-avatar/lime-ai-avatar.scss:0-0
Timestamp: 2025-04-25T14:23:48.175Z
Learning: The `rotate` CSS property (from CSS Transforms Level 2) should be preferred over `transform: rotate()` in cases where multiple independent transformations need to be animated simultaneously to avoid animation conflicts. If all transformations used the `transform` property, they would override each other.
src/components/table/columns.ts (1)
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using '@limetech/lime-elements' rather than defining duplicate types locally. This includes types like `EditorUiType` which are part of the component's public API.
src/components/table/table.tsx (7)
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:1-11
Timestamp: 2025-03-04T14:27:29.714Z
Learning: In example components (typically found in `src/components/*/examples/`), imports of lime-elements exports should use `@limetech/lime-elements` instead of relative paths. This is because example components aren't included in type declarations and are used in documentation to show developers how to properly use lime-elements components.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-16T14:14:18.253Z
Learning: For lime-elements, example files should be self-contained and avoid importing internal implementation details. Example files should only import public exports from '@limetech/lime-elements' or use relative imports for other files within the examples folder. Duplicating simple type definitions in example files is preferred over importing internal types.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3581
File: src/components/chip-set/chip-set.tsx:626-626
Timestamp: 2025-07-02T12:49:21.522Z
Learning: When contributors add new props to existing components in the lime-elements repository, they should always add documentation examples that demonstrate the new prop's usage and explain how it works. This helps with user adoption, feature discoverability, and maintains comprehensive documentation.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-16T14:14:18.253Z
Learning: For lime-elements, example files should be self-contained and avoid importing internal implementation details. This includes not importing types from internal files like '../types.ts', especially those marked with @beta. Duplicating simple type definitions in example files is preferred over importing internal types that may change.
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using '@limetech/lime-elements' rather than defining duplicate types locally. This includes types like `EditorUiType` which are part of the component's public API.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3518
File: src/components/notched-outline/examples/notched-outline-basic.scss:11-22
Timestamp: 2025-04-14T10:01:18.721Z
Learning: Examples in Lime Elements use Shadow DOM (with `shadow: true` in the component decorator) for style encapsulation, making additional class-based selectors unnecessary. This ensures styles are automatically scoped to each component without leaking to the rest of the application.
Learnt from: CR
PR: Lundalogik/lime-elements#0
File: .cursor/rules/wrap-multiple-jsx-elements-in-host-not-in-array.mdc:0-0
Timestamp: 2025-07-01T14:43:09.742Z
Learning: Applies to **/*.{tsx,jsx} : When returning multiple JSX elements from the `render` method, never wrap them in an array literal. Instead, always wrap them in the special `<Host>` element. When there is already a single top-level element, it does not have to be wrapped in `<Host>`.
🧬 Code Graph Analysis (3)
src/components/table/examples/table-pagination.tsx (3)
src/components/table/table.tsx (1)
Component(58-933)src/components/table/table.types.ts (1)
Column(5-45)src/components/select/option.types.ts (1)
Option(7-56)
src/components/table/columns.ts (2)
src/components/table/table.types.ts (2)
Column(5-45)ColumnSorter(122-132)src/components/table/element-pool.ts (1)
ElementPool(1-67)
src/components/table/table.tsx (1)
src/components/table/table.types.ts (2)
ColumnSorter(122-132)Column(5-45)
🪛 Biome (1.9.4)
src/components/table/table.tsx
[error] 221-221: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 548-548: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (25)
src/components/table/partial-styles/_table-main-layout.scss (1)
19-27: No remaining.tabulator-tableHolderreferences
Search across the codebase found only the new lowercase.tabulator-tableholder, confirming the old selector has been fully replaced.src/components/table/table.scss (1)
52-57: Selector rename looks goodAlignment with Tabulator 6’s
.tabulator-tableholderis correct.src/components/table/partial-styles/tabulator-custom-styles.scss (1)
164-170::has()selector – double-check Firefox & old Safari supportThe
:has()pseudo-class is still absent in Firefox ESR and some enterprise browsers.
If table pagination placement is critical, provide a graceful fallback (e.g., additional host class toggled via JS).src/components/table/table-selection.spec.ts (2)
1-1: LGTM: Consistent import refactoring for Tabulator upgradeThe change from default import to named imports aligns with the broader refactor to use explicit Tabulator type imports throughout the codebase.
44-44: LGTM: Type annotations updated correctlyThe type annotations for
makeCellandmakeRowfunctions are properly updated to use the importedCellComponentandRowComponenttypes directly, maintaining consistency with the new import structure.Also applies to: 60-63
src/components/table/table-selection.ts (2)
2-7: LGTM: Well-structured import refactoringThe migration from default import to named imports with clear aliases (e.g.,
TabulatorCellComponent,TabulatorRowComponent) improves type clarity and aligns with the Tabulator version 6 upgrade.
140-142: LGTM: Good defensive programming practiceThe guard clause prevents further processing when
getPosition(true)returnsfalse, which is a good safety measure for edge cases in the row selection logic.src/components/table/partial-styles/tabulator-arrow.scss (3)
3-12: LGTM: Better CSS organization with nested selectorsThe nested structure provides better specificity and organization for the tabulator column sorter styles, aligning with the Tabulator version 6 upgrade.
15-47: LGTM: Improved opacity and hover state handlingThe restructured opacity controls with nested selectors provide cleaner separation between different sorting states (
none,desc,asc) and their respective hover behaviors.
50-50: LGTM: Centralized arrow rotationMoving the
rotate: 180degproperty directly to.tabulator-arrowcentralizes the arrow direction styling, making it cleaner than applying rotation in individual state transforms.src/components/table/examples/table-pagination.tsx (3)
1-8: LGTM: Correct imports following coding guidelinesThe imports correctly use
@limetech/lime-elementsfor public API types and components, following the established pattern for example files in the lime-elements project.
19-23: LGTM: Proper StencilJS component structureThe component follows StencilJS conventions correctly with proper decorators, shadow DOM usage, and component metadata.
44-63: LGTM: Effective pagination demonstrationThe render method clearly demonstrates the pagination functionality with both the table component and interactive controls, wrapped correctly in the
<Host>element as per StencilJS guidelines.src/components/table/columns.ts (2)
2-9: LGTM: Consistent Tabulator type importsThe migration to named imports from
tabulator-tablesprovides better type clarity and consistency with the broader codebase refactor for the Tabulator version 6 upgrade.
25-26: LGTM: Comprehensive type annotation updatesAll function signatures and type annotations are properly updated to use the imported Tabulator types directly, maintaining type safety while improving clarity. The changes are consistent across all affected functions.
Also applies to: 92-92, 134-134, 159-159, 239-239, 274-274, 284-284
src/components/table/table.tsx (10)
11-24: LGTM! Proper migration to Tabulator v6 named imports.The refactoring from default import to named imports aligns with Tabulator v6's export structure and improves tree-shaking capabilities.
46-46: Good addition of pagination example documentation.Adding the example component reference helps developers discover and understand the new pagination location feature.
293-295: Good defensive check for table initialization.Adding the
initializedcheck prevents potential race conditions where data updates are attempted before the table is fully ready.
448-448: Proper state management during table destruction.Resetting the
initializedflag ensures the component state accurately reflects the table's lifecycle.
487-504: Excellent refactoring of tabulator initialization logic.The
createTabulatormethod properly encapsulates table creation, event handler setup, and initialization flow. Using thetableBuiltevent to manage theinitializedflag ensures data operations only occur after the table is fully constructed.
529-529: Proper type updates for Tabulator v6 compatibility.The method signatures now use specific Tabulator types instead of generic ones, improving type safety and API alignment.
Also applies to: 547-547, 555-562, 564-564
658-669: Clean pagination configuration for Tabulator v6.The updated pagination options with explicit
pagination: trueandpaginationModeprovide clearer intent and align with Tabulator v6's API structure.
742-744: Proper event propagation handling.Checking
defaultPreventedallows child elements to stop row activation when needed, improving component flexibility.
859-859: Proper implementation of pagination location rendering.The render method correctly:
- Calculates total rows for accurate pagination display logic
- Applies CSS class based on
paginationLocationprop- Shows pagination based on actual row count vs page size
Also applies to: 865-865, 871-871
174-178: Pagination location styling confirmedThe
paginationLocationprop is correctly typed ('top' | 'bottom'), reflected as an attribute, and defaults to'bottom'. Custom SCSS covers the top placement, and the bottom placement falls back to Tabulator’s default styling.• src/components/table/table.tsx – binds the class
has-pagination-on-topwhenpaginationLocation === 'top.
• src/components/table/partial-styles/tabulator-custom-styles.scss – defines the CSS rules under:host(.has-pagination-on-top)to reorder header/pagination.Approving these changes.
jgroth
force-pushed
the
upgrade-tabulator
branch
from
aed856a to
5b6b9fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/components/table/table.scss (1)
72-78: Over-specific compound selector may miss future Tabulator changes
Coupling hover styles to both.tabulator-sortableand.tabulator-col-sorter-elementrisks losing the hover colour if Tabulator refactors its internal class list (it already changed once in v6). Prefer targeting.tabulator-sortableonly, or add a fallback rule.
♻️ Duplicate comments (4)
src/components/table/partial-styles/_row-selection.scss (1)
18-18: Magic number4pxis still hard-coded
The earlier feedback about avoiding literal offsets hasn’t been addressed. If density or checkbox size changes, this value drifts again. Consider deriving it from$cell-paddingor the checkbox size viacalc()instead offunctions.pxToRem(4).src/components/table/partial-styles/tabulator-custom-styles.scss (1)
149-152: Selector rename – ensure no orphaned styles
Same caveat as before: verify there are no remaining.tabulator-tableHolderselectors elsewhere.src/components/table/table.tsx (2)
221-221: Remove redundant type annotation.The type annotation is unnecessary as TypeScript infers the boolean type from the initialization value.
621-624: Use optional chaining for safer property access.The current implementation could throw if
tabulatoris null. Optional chaining provides a safer approach.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.mdpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
package.json(2 hunks)src/components/table/columns.ts(9 hunks)src/components/table/examples/table-pagination.tsx(1 hunks)src/components/table/partial-styles/_row-selection.scss(1 hunks)src/components/table/partial-styles/_table-main-layout.scss(1 hunks)src/components/table/partial-styles/tabulator-arrow.scss(4 hunks)src/components/table/partial-styles/tabulator-custom-styles.scss(2 hunks)src/components/table/table-selection.spec.ts(3 hunks)src/components/table/table-selection.ts(6 hunks)src/components/table/table.scss(3 hunks)src/components/table/table.tsx(20 hunks)types/tabulator-tables/index.d.ts(0 hunks)
💤 Files with no reviewable changes (1)
- types/tabulator-tables/index.d.ts
🧰 Additional context used
📓 Path-based instructions (6)
`src/components/**/*.scss`: Do not use BEM-style class names in component CSS files, as components use shadow-DOM.
src/components/**/*.scss: Do not use BEM-style class names in component CSS files, as components use shadow-DOM.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/css-class-names-in-components-using-shadow-dom.mdc)
List of files the instruction was applied to:
src/components/table/partial-styles/_table-main-layout.scsssrc/components/table/partial-styles/_row-selection.scsssrc/components/table/table.scsssrc/components/table/partial-styles/tabulator-custom-styles.scsssrc/components/table/partial-styles/tabulator-arrow.scss
`**/*.{tsx,scss}`: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/components/table/partial-styles/_table-main-layout.scsssrc/components/table/partial-styles/_row-selection.scsssrc/components/table/table.scsssrc/components/table/partial-styles/tabulator-custom-styles.scsssrc/components/table/partial-styles/tabulator-arrow.scsssrc/components/table/examples/table-pagination.tsxsrc/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.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/components/table/table-selection.spec.tssrc/components/table/table-selection.tssrc/components/table/examples/table-pagination.tsxsrc/components/table/columns.tssrc/components/table/table.tsx
`**/*.{tsx,jsx}`: When returning multiple JSX elements from the `render` method,...
**/*.{tsx,jsx}: When returning multiple JSX elements from therendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element. When there is already a single top-level element, it does not have to be wrapped in<Host>.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/wrap-multiple-jsx-elements-in-host-not-in-array.mdc)
List of files the instruction was applied to:
src/components/table/examples/table-pagination.tsxsrc/components/table/table.tsx
`**/*.tsx`: Our `.tsx` files are typically using StencilJS, not React. When a de...
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/components/table/examples/table-pagination.tsxsrc/components/table/table.tsx
`src/components/**/examples/**/*.{ts,tsx}`: These files are an exception to the ...
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publically exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/components/table/examples/table-pagination.tsx
🧠 Learnings (10)
src/components/table/partial-styles/_table-main-layout.scss (1)
Learnt from: CR
PR: Lundalogik/lime-elements#0
File: .cursor/rules/css-class-names-in-components-using-shadow-dom.mdc:0-0
Timestamp: 2025-07-01T14:43:04.238Z
Learning: Applies to src/components/**/*.scss : Do not use BEM-style class names in component CSS files, as components use shadow-DOM.
src/components/table/partial-styles/_row-selection.scss (2)
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3539
File: src/components/text-editor/text-editor.scss:14-14
Timestamp: 2025-04-24T10:16:05.891Z
Learning: In SCSS, prefer using `calc()` function over native SCSS arithmetic operations (like `$variable / 2`) as it aligns with modern SASS recommendations for better CSS compatibility and interoperability with CSS variables.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3518
File: src/components/chip-set/chip-set.tsx:367-385
Timestamp: 2025-04-14T12:22:22.298Z
Learning: In limel-chip-set, the onClick handler should be placed on the content div (<div slot="content">) rather than on the limel-notched-outline component, and should only be applied when type is 'input'.
src/components/table/table.scss (1)
Learnt from: CR
PR: Lundalogik/lime-elements#0
File: .cursor/rules/css-class-names-in-components-using-shadow-dom.mdc:0-0
Timestamp: 2025-07-01T14:43:04.238Z
Learning: Applies to src/components/**/*.scss : Do not use BEM-style class names in component CSS files, as components use shadow-DOM.
src/components/table/table-selection.spec.ts (2)
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using '@limetech/lime-elements' rather than defining duplicate types locally. This includes types like `EditorUiType` which are part of the component's public API.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-16T14:14:18.253Z
Learning: For lime-elements, example files should be self-contained and avoid importing internal implementation details. This includes not importing types from internal files like '../types.ts', especially those marked with @beta. Duplicating simple type definitions in example files is preferred over importing internal types that may change.
src/components/table/partial-styles/tabulator-custom-styles.scss (2)
Learnt from: CR
PR: Lundalogik/lime-elements#0
File: .cursor/rules/css-class-names-in-components-using-shadow-dom.mdc:0-0
Timestamp: 2025-07-01T14:43:04.238Z
Learning: Applies to src/components/**/*.scss : Do not use BEM-style class names in component CSS files, as components use shadow-DOM.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3539
File: src/components/text-editor/text-editor.scss:14-14
Timestamp: 2025-04-24T10:16:05.891Z
Learning: In SCSS, prefer using `calc()` function over native SCSS arithmetic operations (like `$variable / 2`) as it aligns with modern SASS recommendations for better CSS compatibility and interoperability with CSS variables.
src/components/table/table-selection.ts (1)
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using '@limetech/lime-elements' rather than defining duplicate types locally. This includes types like `EditorUiType` which are part of the component's public API.
src/components/table/partial-styles/tabulator-arrow.scss (3)
Learnt from: CR
PR: Lundalogik/lime-elements#0
File: .cursor/rules/css-class-names-in-components-using-shadow-dom.mdc:0-0
Timestamp: 2025-07-01T14:43:04.238Z
Learning: Applies to src/components/**/*.scss : Do not use BEM-style class names in component CSS files, as components use shadow-DOM.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3567
File: src/components/ai-avatar/ai-avatar.scss:0-0
Timestamp: 2025-05-28T08:37:45.610Z
Learning: In SCSS files, CSS property values can be split across multiple lines when properly indented, particularly for complex values like animation shorthands with CSS custom properties. This is valid SCSS syntax that compiles correctly to CSS and is often the result of auto-formatting tools.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3529
File: src/components/lime-ai-avatar/lime-ai-avatar.scss:0-0
Timestamp: 2025-04-25T14:23:48.175Z
Learning: The `rotate` CSS property (from CSS Transforms Level 2) should be preferred over `transform: rotate()` in cases where multiple independent transformations need to be animated simultaneously to avoid animation conflicts. If all transformations used the `transform` property, they would override each other.
src/components/table/examples/table-pagination.tsx (4)
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-16T14:14:18.253Z
Learning: For lime-elements, example files should be self-contained and avoid importing internal implementation details. This includes not importing types from internal files like '../types.ts', especially those marked with @beta. Duplicating simple type definitions in example files is preferred over importing internal types that may change.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-16T14:14:18.253Z
Learning: For lime-elements, example files should be self-contained and avoid importing internal implementation details. Example files should only import public exports from '@limetech/lime-elements' or use relative imports for other files within the examples folder. Duplicating simple type definitions in example files is preferred over importing internal types.
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:1-11
Timestamp: 2025-03-04T14:27:29.714Z
Learning: In example components (typically found in `src/components/*/examples/`), imports of lime-elements exports should use `@limetech/lime-elements` instead of relative paths. This is because example components aren't included in type declarations and are used in documentation to show developers how to properly use lime-elements components.
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using '@limetech/lime-elements' rather than defining duplicate types locally. This includes types like `EditorUiType` which are part of the component's public API.
src/components/table/columns.ts (1)
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using '@limetech/lime-elements' rather than defining duplicate types locally. This includes types like `EditorUiType` which are part of the component's public API.
src/components/table/table.tsx (7)
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3547
File: src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx:46-46
Timestamp: 2025-05-02T07:14:19.314Z
Learning: When reviewing TypeScript/JavaScript code, don't suggest converting object literals to constants when the object is mutable and will be modified later in the code. Due to reference semantics, this can cause unintended side effects where the "constant" version is also modified.
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:1-11
Timestamp: 2025-03-04T14:27:29.714Z
Learning: In example components (typically found in `src/components/*/examples/`), imports of lime-elements exports should use `@limetech/lime-elements` instead of relative paths. This is because example components aren't included in type declarations and are used in documentation to show developers how to properly use lime-elements components.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-16T14:14:18.253Z
Learning: For lime-elements, example files should be self-contained and avoid importing internal implementation details. Example files should only import public exports from '@limetech/lime-elements' or use relative imports for other files within the examples folder. Duplicating simple type definitions in example files is preferred over importing internal types.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3581
File: src/components/chip-set/chip-set.tsx:626-626
Timestamp: 2025-07-02T12:49:21.522Z
Learning: When contributors add new props to existing components in the lime-elements repository, they should always add documentation examples that demonstrate the new prop's usage and explain how it works. This helps with user adoption, feature discoverability, and maintains comprehensive documentation.
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-16T14:14:18.253Z
Learning: For lime-elements, example files should be self-contained and avoid importing internal implementation details. This includes not importing types from internal files like '../types.ts', especially those marked with @beta. Duplicating simple type definitions in example files is preferred over importing internal types that may change.
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using '@limetech/lime-elements' rather than defining duplicate types locally. This includes types like `EditorUiType` which are part of the component's public API.
Learnt from: CR
PR: Lundalogik/lime-elements#0
File: .cursor/rules/wrap-multiple-jsx-elements-in-host-not-in-array.mdc:0-0
Timestamp: 2025-07-01T14:43:09.742Z
Learning: Applies to **/*.{tsx,jsx} : When returning multiple JSX elements from the `render` method, never wrap them in an array literal. Instead, always wrap them in the special `<Host>` element. When there is already a single top-level element, it does not have to be wrapped in `<Host>`.
🧬 Code Graph Analysis (2)
src/components/table/columns.ts (2)
src/components/table/table.types.ts (2)
Column(5-45)ColumnSorter(122-132)src/components/table/element-pool.ts (1)
ElementPool(1-67)
src/components/table/table.tsx (1)
src/components/table/table.types.ts (2)
ColumnSorter(122-132)Column(5-45)
🪛 Biome (1.9.4)
src/components/table/table.tsx
[error] 221-221: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 548-548: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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). (3)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Build
- GitHub Check: Test
🔇 Additional comments (20)
src/components/table/partial-styles/_table-main-layout.scss (1)
19-27: ✅ Renamed selector verified – no stale references found
A global search for both.tabulator-tableHolderandtabulatorTableHolderyielded no matches in TSX/JS/SCSS files. The selector rename to.tabulator-tableholderis safe to merge.src/components/table/table.scss (1)
52-57: Verify Tabulator 6 table holder CSS class usage
Grep didn’t return any matches (the Tabulator bundle isn’t in this repo), so we need to confirm whether Tabulator 6 emits.tabulator-tableholder(all-lowercase) or still uses.tabulator-tableHolder. A mismatch will cause your theming rule to be ignored and you’ll lose text-color styling.
- Location to verify:
• src/components/table/table.scss (Lines 52–57).tabulator-tableholder { .tabulator-table { color: var(--table-text-color); background-color: transparent; } }- Suggested checks:
# Inspect installed Tabulator package grep -R "tableholder" node_modules/tabulator-tables grep -R "tableHolder" node_modules/tabulator-tables- Alternatively, open your app in the browser, inspect the table holder element in DevTools, and note the exact class name.
Ensure your SCSS selector matches the emitted class to preserve theming.
package.json (1)
61-61: ESM-Only Upgrade Verified—No Changes Needed
We’ve confirmed there are no default/namespace imports oftabulator-tablesin the codebase and that@rollup/plugin-node-resolveis enabled in all stencil configs (stencil.config.ts,.dist.ts,.docs.ts). The ESM-only upgrade totabulator-tables@6can proceed without additional toolchain changes.src/components/table/table-selection.spec.ts (2)
1-1: LGTM: Import structure updated for Tabulator v6The change from namespace imports to explicit named imports aligns with the broader refactoring to upgrade Tabulator from v4 to v6. This improves type clarity and follows the new package structure.
44-44: Type annotations correctly updatedThe type annotations have been properly updated to use the imported types directly (
CellComponent,RowComponent) instead of the namespace-qualified versions (Tabulator.CellComponent,Tabulator.RowComponent).Also applies to: 60-63
src/components/table/table-selection.ts (3)
2-7: LGTM: Import structure improved with explicit aliasesThe change from default import to named imports with "Tabulator" prefixes improves type clarity and prevents naming conflicts. This aligns with the broader refactoring for Tabulator v6 upgrade.
87-88: Type annotations correctly updatedAll type annotations have been properly updated to use the new imported type aliases (
TabulatorColumnDefinition,TabulatorCellComponent,TabulatorRowComponent), maintaining consistency throughout the file.Also applies to: 92-92, 111-111, 132-132, 172-172, 182-182
140-142: Good defensive programming with guard clauseThe guard clause prevents further processing when
getPosition(true)returnsfalse, which can happen when the row is not found or in certain edge cases. This is a good defensive programming practice.src/components/table/partial-styles/tabulator-arrow.scss (4)
3-12: CSS selector restructuring looks goodThe nesting structure has been improved and the removal of
!importantflags fromrightandpositionproperties suggests better CSS specificity management. This aligns with the Tabulator v6 upgrade requirements.
14-47: Opacity control restructuring improves organizationThe opacity control has been better organized by moving it inside
.tabulator-sortablewith clearer separation between different sorting states. The nested structure makes the relationship between states more explicit.
49-50: Centralized arrow rotation is a good approachAdding a global
rotate: 180degto.tabulator-arrowcentralizes the rotation logic, which is cleaner than having rotation defined in each individual state. This follows the DRY principle and makes the styling more maintainable.
78-78: Transform simplification aligns with centralized rotationRemoving
rotate(180deg)from individual state transforms and keeping only thetranslate3doperations is consistent with the centralized rotation approach. This simplifies the transform rules and reduces redundancy.Also applies to: 89-89
src/components/table/examples/table-pagination.tsx (2)
1-8: Imports correctly follow example file guidelinesThe imports properly use
@limetech/lime-elementsfor public API types and components, which aligns with the coding guidelines for example files. The relative import for./birdsis appropriate for shared example data.
19-76: Well-structured example componentThe component follows established Stencil patterns with proper decorators, state management, and event handling. It effectively demonstrates the new pagination functionality with both
pageSizeandpaginationLocationproperties, providing a clear example of how to use these features.src/components/table/columns.ts (2)
2-9: Import structure properly updated for Tabulator v6The change from namespace imports to explicit named imports improves type clarity and aligns with the Tabulator v6 upgrade pattern. Using
SorterFromTableinstead of a customTabulatorSorterinterface reduces custom type definitions in favor of official types.
25-26: Type annotations consistently updatedAll function signatures and type annotations have been properly updated to use the imported Tabulator types directly. This maintains type safety while leveraging the official type definitions from the upgraded package.
Also applies to: 92-92, 108-108, 134-134, 159-159, 239-239, 274-274, 284-284
src/components/table/table.tsx (4)
11-24: LGTM! Good refactoring to use explicit named imports.The change from generic imports to specific named imports from 'tabulator-tables' improves type safety, enables better tree-shaking, and makes dependencies more explicit.
487-504: Good refactoring of Tabulator initialization logic.Extracting the table creation into a separate method improves code organization and makes the initialization flow clearer. The proper management of the
initializedflag in the 'tableBuilt' event ensures components know when the table is ready.
742-744: Good addition of event propagation check.Checking
event.defaultPreventedbefore processing the row click is a best practice that respects event handling by other components and prevents unintended activations.
859-871: Proper implementation of pagination location styling.The render method correctly implements the pagination location feature by:
- Adding the appropriate CSS class based on the
paginationLocationproperty- Properly calculating
totalRowsfor pagination visibility- Following the existing pattern for conditional CSS classes
src/components/table/partial-styles/tabulator-custom-styles.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades the Tabulator table library from version 4 to version 6, introducing new features and improving the table component's functionality.
- Updated Tabulator dependency from 4.9.3 to 6.3.1 with corresponding type definitions
- Added pagination location control allowing positioning of pagination controls at top or bottom
- Enhanced table initialization handling and improved type safety throughout the codebase
Reviewed Changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/components/table/table.tsx |
Updated imports, types, and initialization logic for Tabulator v6 compatibility; added pagination location feature |
src/components/table/table.scss |
Fixed CSS class name and adjusted column header padding |
src/components/table/table-selection.ts |
Updated imports and type definitions for Tabulator v6 |
src/components/table/table-selection.spec.ts |
Updated test types for Tabulator v6 compatibility |
src/components/table/partial-styles/tabulator-custom-styles.scss |
Fixed CSS class name and adjusted select-all checkbox positioning |
src/components/table/partial-styles/tabulator-arrow.scss |
Refactored sorting arrow styles and fixed arrow direction |
src/components/table/partial-styles/_table-main-layout.scss |
Fixed CSS class name reference |
src/components/table/partial-styles/_row-selection.scss |
Adjusted select-all checkbox positioning |
src/components/table/examples/table-pagination.tsx |
Added new example demonstrating pagination location control |
src/components/table/columns.ts |
Updated imports and type definitions for Tabulator v6 |
package.json |
Updated Tabulator dependencies to version 6 |
etc/lime-elements.api.md |
Added API documentation for new pagination location property |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/table/table.tsx (2)
489-506: Good refactoring of initialization logic with one issue.The extraction of tabulator creation improves code organization. However, the hardcoded URL on line 499 should use a configured value.
if (this.isRemoteMode()) { - this.tabulator.setData('https://localhost'); + const remoteUrl = 'https://localhost'; // This should match the URL used in getAjaxOptions + this.tabulator.setData(remoteUrl); } else {
623-626: Convert ajaxParams to a function for safer initialization.The current implementation may evaluate
this.tabulatormethods before the instance is ready. Converting to a function ensures lazy evaluation.- ajaxParams: { - page: this.tabulator?.getPage() || FIRST_PAGE, - sorters: this.tabulator?.getSorters() || [], - }, + ajaxParams: () => ({ + page: this.tabulator?.getPage() || FIRST_PAGE, + sorters: this.tabulator?.getSorters() || [], + }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/components/table/table.tsx(20 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/wrap-multiple-jsx-elements-in-host-not-in-array.mdc)
When returning multiple JSX elements from the
rendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element. When there is already a single top-level element, it does not have to be wrapped in<Host>.
Files:
src/components/table/table.tsx
⚙️ CodeRabbit Configuration File
Our
.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
Files:
src/components/table/table.tsx
**/*.{ts,tsx}
⚙️ CodeRabbit Configuration File
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.
Files:
src/components/table/table.tsx
**/*.{tsx,scss}
⚙️ CodeRabbit Configuration File
Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
Files:
src/components/table/table.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: Kiarokh
PR: Lundalogik/lime-elements#3478
File: package.json:61-61
Timestamp: 2025-07-28T14:35:21.933Z
Learning: In Tabulator 6, the SCSS files are located in `dist/scss/tabulator.scss`, not in the old `src/scss/` path from version 4.x. The correct import path is `node_modules/tabulator-tables/dist/scss/tabulator.scss`.
src/components/table/table.tsx (9)
Learnt from: Kiarokh
PR: #3478
File: package.json:61-61
Timestamp: 2025-07-28T14:35:21.933Z
Learning: In Tabulator 6, the SCSS files are located in dist/scss/tabulator.scss, not in the old src/scss/ path from version 4.x. The correct import path is node_modules/tabulator-tables/dist/scss/tabulator.scss.
Learnt from: adrianschmidt
PR: #3547
File: src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx:46-46
Timestamp: 2025-05-02T07:14:19.314Z
Learning: When reviewing TypeScript/JavaScript code, don't suggest converting object literals to constants when the object is mutable and will be modified later in the code. Due to reference semantics, this can cause unintended side effects where the "constant" version is also modified.
Learnt from: Kiarokh
PR: #3581
File: src/components/chip-set/chip-set.tsx:626-626
Timestamp: 2025-07-02T12:49:21.522Z
Learning: When contributors add new props to existing components in the lime-elements repository, they should always add documentation examples that demonstrate the new prop's usage and explain how it works. This helps with user adoption, feature discoverability, and maintains comprehensive documentation.
Learnt from: adrianschmidt
PR: #3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:1-11
Timestamp: 2025-03-04T14:27:29.714Z
Learning: In example components (typically found in src/components/*/examples/), imports of lime-elements exports should use @limetech/lime-elements instead of relative paths. This is because example components aren't included in type declarations and are used in documentation to show developers how to properly use lime-elements components.
Learnt from: Kiarokh
PR: #3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-16T14:14:18.253Z
Learning: For lime-elements, example files should be self-contained and avoid importing internal implementation details. Example files should only import public exports from '@limetech/lime-elements' or use relative imports for other files within the examples folder. Duplicating simple type definitions in example files is preferred over importing internal types.
Learnt from: Kiarokh
PR: #3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-16T14:14:18.253Z
Learning: For lime-elements, example files should be self-contained and avoid importing internal implementation details. This includes not importing types from internal files like '../types.ts', especially those marked with @beta. Duplicating simple type definitions in example files is preferred over importing internal types that may change.
Learnt from: adrianschmidt
PR: #3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using '@limetech/lime-elements' rather than defining duplicate types locally. This includes types like EditorUiType which are part of the component's public API.
Learnt from: Kiarokh
PR: #3518
File: src/components/notched-outline/examples/notched-outline-basic.scss:11-22
Timestamp: 2025-04-14T10:01:18.721Z
Learning: Examples in Lime Elements use Shadow DOM (with shadow: true in the component decorator) for style encapsulation, making additional class-based selectors unnecessary. This ensures styles are automatically scoped to each component without leaking to the rest of the application.
Learnt from: CR
PR: Lundalogik/lime-elements#0
File: .cursor/rules/wrap-multiple-jsx-elements-in-host-not-in-array.mdc:0-0
Timestamp: 2025-07-21T00:28:01.726Z
Learning: Applies to **/*.tsx : When returning multiple JSX elements from the render method, never wrap them in an array literal. Instead, always wrap them in the special <Host> element. When there is already a single top-level element, it does not have to be wrapped in <Host>.
🔇 Additional comments (9)
src/components/table/table.tsx (9)
11-24: Import changes align with Tabulator 6.x module structure.The migration from default import to named imports is correct and required for Tabulator 6.x compatibility.
46-46: Good addition of example component for the new pagination feature.Adding the pagination example follows best practices for documenting new component features.
174-180: Well-documented pagination location property.The property is properly documented with clear JSDoc comments and appropriate decorator configuration.
294-297: Good defensive check for initialization state.Adding the
initializedflag check prevents potential race conditions during table initialization.
531-531: Proper type annotations for Tabulator 6.x.All method signatures have been correctly updated with explicit Tabulator types, improving type safety.
Also applies to: 549-549, 557-561, 566-566, 609-609, 660-660, 710-710, 738-738
666-667: Correct pagination configuration for Tabulator 6.x.The explicit
pagination: trueandpaginationModesettings align with Tabulator 6.x API requirements.
744-746: Good practice: respecting event.defaultPrevented.Checking
defaultPreventedallows proper event handling composition with other components.
842-846: Appropriate guard clause for column movement.The early return prevents unnecessary processing when column movement is disabled.
861-873: Render method correctly implements pagination location feature.The CSS class addition and totalRows calculation properly support the new pagination positioning functionality.
jgroth
force-pushed
the
upgrade-tabulator
branch
from
57d8912 to
f732054
Compare
jgroth
force-pushed
the
upgrade-tabulator
branch
2 times, most recently
from
9cf3a8c to
9753dfd
Compare
Kiarokh
force-pushed
the
upgrade-tabulator
branch
from
9753dfd to
6e5f599
Compare
| updateOrAddData: jest.fn(), | ||
| }; | ||
| (component as any).setSelection = jest.fn(); | ||
| (component as any).initialized = true; |
There was a problem hiding this comment.
@jgroth
All tests are now passing now! Do you still have problems on your device?
According to the intelligent fellas, the issue was that during the Tabulator upgrade, a new initialized property was added to the Table class, and the updateData method now checks for this property before executing:
if (!this.tabulator || !this.initialized) {
return;
}The tests were setting up the mock tabulator object but not setting initialized = true (table.tsx line 240) , so all the tabulator method calls were being skipped due to the early return.
The fix was to add (component as any).initialized = true; to the beforeEach block in the test file.
Kiarokh
force-pushed
the
upgrade-tabulator
branch
2 times, most recently
from
152c50d to
33b9c73
Compare
|
@coderabbitai please ignore the old "Copilot" review comments which are in this PR, and do a new fresh review from scratch |
|
@Kiarokh: I'll perform a fresh, comprehensive review of this PR from scratch, focusing on the Tabulator v6 update and all the related changes. I'll ignore the previous automated review comments and provide new feedback on the entire changeset. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/table/table.scss (1)
3-3: Update SCSS import path for Tabulator v6.The import path uses the deprecated v4.x location. For Tabulator 6, change the import to:
@import '../../../node_modules/tabulator-tables/dist/scss/tabulator.scss';src/components/table/table.tsx (1)
797-803: Mark arrow function members asreadonly.SonarCloud correctly identifies that
getActiveRows,getColumnOptions,handleMoveColumn, andfindColumnare never reassigned. Marking them asreadonlyprevents accidental reassignment.- private getActiveRows: () => TabulatorRowComponent[] = () => { + private readonly getActiveRows: () => TabulatorRowComponent[] = () => {Apply similar changes to
getColumnOptions(line 873),handleMoveColumn(line 883), andfindColumn(line 892).src/components/table/table-selection.ts (1)
184-191: Mark asreadonlyfor consistency.Same as in
table.tsx, this arrow function member should be markedreadonlyto prevent accidental reassignment.- private getActiveRows: () => TabulatorRowComponent[] = () => { + private readonly getActiveRows: () => TabulatorRowComponent[] = () => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.mdpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
package.json(2 hunks)src/components/table/columns.ts(9 hunks)src/components/table/examples/table-pagination.tsx(1 hunks)src/components/table/partial-styles/_table-main-layout.scss(1 hunks)src/components/table/partial-styles/tabulator-arrow.scss(4 hunks)src/components/table/partial-styles/tabulator-custom-styles.scss(5 hunks)src/components/table/table-selection.spec.ts(3 hunks)src/components/table/table-selection.ts(6 hunks)src/components/table/table.scss(3 hunks)src/components/table/table.spec.ts(1 hunks)src/components/table/table.tsx(20 hunks)types/tabulator-tables/index.d.ts(0 hunks)
💤 Files with no reviewable changes (1)
- types/tabulator-tables/index.d.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/components/**/*.scss
📄 CodeRabbit inference engine (.cursor/rules/css-class-names-in-components-using-shadow-dom.mdc)
Do not use BEM-style class names in CSS for components, as components use shadow-DOM.
Files:
src/components/table/partial-styles/_table-main-layout.scsssrc/components/table/partial-styles/tabulator-arrow.scsssrc/components/table/partial-styles/tabulator-custom-styles.scsssrc/components/table/table.scss
**/*.{tsx,scss}
⚙️ CodeRabbit configuration file
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
Files:
src/components/table/partial-styles/_table-main-layout.scsssrc/components/table/partial-styles/tabulator-arrow.scsssrc/components/table/examples/table-pagination.tsxsrc/components/table/partial-styles/tabulator-custom-styles.scsssrc/components/table/table.tsxsrc/components/table/table.scss
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
**/*.{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.
Files:
src/components/table/table.spec.tssrc/components/table/table-selection.spec.tssrc/components/table/examples/table-pagination.tsxsrc/components/table/table.tsxsrc/components/table/table-selection.tssrc/components/table/columns.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/wrap-multiple-jsx-elements-in-host-not-in-array.mdc)
When returning multiple JSX elements from the
rendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element. When there is already a single top-level element, it does not have to be wrapped in<Host>.
Files:
src/components/table/examples/table-pagination.tsxsrc/components/table/table.tsx
⚙️ CodeRabbit configuration file
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
Files:
src/components/table/examples/table-pagination.tsxsrc/components/table/table.tsx
src/components/**/examples/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publicly exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
Files:
src/components/table/examples/table-pagination.tsx
src/components/**/*.tsx
⚙️ CodeRabbit configuration file
src/components/**/*.tsx: When contributors add new props to existing components in the lime-elements repository, they should always add documentation examples that demonstrate the new prop's usage and explain how it works. This helps with user adoption, feature discoverability, and maintains comprehensive documentation.
Files:
src/components/table/examples/table-pagination.tsxsrc/components/table/table.tsx
🧠 Learnings (13)
📓 Common learnings
Learnt from: Kiarokh
Repo: Lundalogik/lime-elements PR: 3478
File: package.json:61-61
Timestamp: 2025-07-28T14:35:21.965Z
Learning: In Tabulator 6, the SCSS files are located in `dist/scss/tabulator.scss`, not in the old `src/scss/` path from version 4.x. The correct import path is `node_modules/tabulator-tables/dist/scss/tabulator.scss`.
📚 Learning: 2025-07-28T14:35:21.965Z
Learnt from: Kiarokh
Repo: Lundalogik/lime-elements PR: 3478
File: package.json:61-61
Timestamp: 2025-07-28T14:35:21.965Z
Learning: In Tabulator 6, the SCSS files are located in `dist/scss/tabulator.scss`, not in the old `src/scss/` path from version 4.x. The correct import path is `node_modules/tabulator-tables/dist/scss/tabulator.scss`.
Applied to files:
src/components/table/partial-styles/_table-main-layout.scsssrc/components/table/table-selection.spec.tssrc/components/table/partial-styles/tabulator-arrow.scsspackage.jsonsrc/components/table/partial-styles/tabulator-custom-styles.scsssrc/components/table/table.tsxsrc/components/table/table-selection.tssrc/components/table/columns.tssrc/components/table/table.scss
📚 Learning: 2025-03-04T14:27:29.714Z
Learnt from: adrianschmidt
Repo: Lundalogik/lime-elements PR: 3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:1-11
Timestamp: 2025-03-04T14:27:29.714Z
Learning: In example components (typically found in `src/components/*/examples/`), imports of lime-elements exports should use `limetech/lime-elements` instead of relative paths. This is because example components aren't included in type declarations and are used in documentation to show developers how to properly use lime-elements components.
Applied to files:
package.jsonsrc/components/table/table.tsx
📚 Learning: 2025-03-04T13:48:44.712Z
Learnt from: adrianschmidt
Repo: Lundalogik/lime-elements PR: 3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:1-11
Timestamp: 2025-03-04T13:48:44.712Z
Learning: The lime-elements codebase must use relative imports (even with multiple "../") rather than alias imports or re-exports because they export types to `<repo-root>/dist/types`. Using non-relative imports breaks type generation as import paths from generated type files point to full .ts files rather than .d.ts files.
Applied to files:
package.json
📚 Learning: 2025-11-27T12:31:25.573Z
Learnt from: Kiarokh
Repo: Lundalogik/lime-elements PR: 0
File: :0-0
Timestamp: 2025-11-27T12:31:25.573Z
Learning: In lime-elements, custom CSS properties (e.g., `--example-controls-column-layout`) must be treated as component APIs. Before removing or renaming any CSS custom property, all usage instances must be identified and addressed in the same PR, with backwards compatibility implemented if needed.
Applied to files:
src/components/table/partial-styles/tabulator-custom-styles.scss
📚 Learning: 2025-11-27T12:31:25.573Z
Learnt from: Kiarokh
Repo: Lundalogik/lime-elements PR: 0
File: :0-0
Timestamp: 2025-11-27T12:31:25.573Z
Learning: In lime-elements code reviews, flag any removal of CSS custom properties (including those with `var(--property-name, fallback)` syntax) that doesn't include corresponding updates to all usage instances or backwards compatibility implementation in the same PR.
Applied to files:
src/components/table/partial-styles/tabulator-custom-styles.scss
📚 Learning: 2025-04-25T14:22:02.157Z
Learnt from: Kiarokh
Repo: Lundalogik/lime-elements PR: 3529
File: src/components/lime-ai-avatar/lime-ai-avatar.scss:24-28
Timestamp: 2025-04-25T14:22:02.157Z
Learning: In lime-elements, CSS custom properties follow a specific naming convention:
1. Internal use variables (not for consumers) are prefixed with `--limel-component-name-`
2. External use variables (intended for consumers) are prefixed with `--component-name-`
Variables documented with JSDoc comments are typically meant for external use.
Applied to files:
src/components/table/partial-styles/tabulator-custom-styles.scss
📚 Learning: 2025-07-02T12:49:21.522Z
Learnt from: Kiarokh
Repo: Lundalogik/lime-elements PR: 3581
File: src/components/chip-set/chip-set.tsx:626-626
Timestamp: 2025-07-02T12:49:21.522Z
Learning: When contributors add new props to existing components in the lime-elements repository, they should always add documentation examples that demonstrate the new prop's usage and explain how it works. This helps with user adoption, feature discoverability, and maintains comprehensive documentation.
Applied to files:
src/components/table/table.tsx
📚 Learning: 2025-08-21T15:57:46.879Z
Learnt from: Kiarokh
Repo: Lundalogik/lime-elements PR: 3628
File: src/components/list-item/examples/list-item-actions.tsx:40-63
Timestamp: 2025-08-21T15:57:46.879Z
Learning: The examples in src/components/list-item/examples/ are for internal usage and verification, not for external developers. The actual consumers of limel-list-item are other internal components like limel-list or limel-menu-list. These examples should be taken with a pinch of salt regarding semantic correctness and accessibility, as the proper implementation will be handled by the consuming components.
Applied to files:
src/components/table/table.tsx
📚 Learning: 2025-08-26T08:42:17.994Z
Learnt from: Kiarokh
Repo: Lundalogik/lime-elements PR: 3628
File: src/components/list-item/list-item.tsx:316-321
Timestamp: 2025-08-26T08:42:17.994Z
Learning: In src/components/list-item/list-item.tsx, the RadioButtonTemplate and CheckboxTemplate components are used purely for visualization and receive their checked state from the parent component's selected prop. The actual interaction is handled by a centralized onClick handler on the Host element, making the entire list item clickable. Adding onChange handlers to these templates would be redundant and potentially problematic.
Applied to files:
src/components/table/table-selection.ts
📚 Learning: 2025-04-24T10:16:05.891Z
Learnt from: Kiarokh
Repo: Lundalogik/lime-elements PR: 3539
File: src/components/text-editor/text-editor.scss:14-14
Timestamp: 2025-04-24T10:16:05.891Z
Learning: In SCSS, prefer using `calc()` function over native SCSS arithmetic operations (like `$variable / 2`) as it aligns with modern SASS recommendations for better CSS compatibility and interoperability with CSS variables.
Applied to files:
src/components/table/table.scss
📚 Learning: 2025-07-30T14:20:41.791Z
Learnt from: Kiarokh
Repo: Lundalogik/lime-elements PR: 3624
File: src/components/button-group/button-group.scss:115-118
Timestamp: 2025-07-30T14:20:41.791Z
Learning: In lime-elements, CSS custom properties like `--limel-theme-surface-background-color` are applied at :root level via `core-styles.scss` and should be expected to be available without fallbacks. The library's architecture guarantees these theme variables are present, so adding fallbacks would be redundant and go against the design intention.
Applied to files:
src/components/table/table.scss
📚 Learning: 2025-10-12T21:44:29.530Z
Learnt from: adrianschmidt
Repo: Lundalogik/lime-elements PR: 3703
File: context7.json:24-24
Timestamp: 2025-10-12T21:44:29.530Z
Learning: In the lime-elements repository, CSS custom properties follow a two-tier naming convention: `--lime-*` properties are public API intended for external consumers and are documented in theming guides (e.g., `--lime-primary-color`, `--lime-on-primary-color`), while `--limel-*` properties are internal implementation details used as fallbacks and are not documented for public use (e.g., `--limel-theme-primary-color`). The `--lime-` prefix is used specifically to avoid naming conflicts with third-party code.
Applied to files:
src/components/table/table.scss
🧬 Code graph analysis (3)
src/components/table/examples/table-pagination.tsx (3)
src/components/table/table.tsx (1)
Component(61-982)src/components/table/table.types.ts (1)
Column(5-45)src/components/select/option.types.ts (1)
Option(7-56)
src/components/table/table.tsx (1)
src/components/table/table.types.ts (2)
ColumnSorter(122-132)Column(5-45)
src/components/table/columns.ts (2)
src/components/table/table.types.ts (2)
Column(5-45)ColumnSorter(122-132)src/components/table/element-pool.ts (1)
ElementPool(1-67)
🪛 GitHub Check: SonarCloud Code Analysis
src/components/table/examples/table-pagination.tsx
[warning] 42-42: Member 'pageSize' is never reassigned; mark it as readonly.
[warning] 37-37: Member 'paginationLocationOptions' is never reassigned; mark it as readonly.
[warning] 71-71: Member 'handlePaginationLocationChange' is never reassigned; mark it as readonly.
[warning] 65-65: Member 'getSelectedPaginationLocation' is never reassigned; mark it as readonly.
[warning] 26-26: Member 'columns' is never reassigned; mark it as readonly.
src/components/table/table.tsx
[warning] 797-797: Member 'getActiveRows' is never reassigned; mark it as readonly.
[warning] 873-873: Member 'getColumnOptions' is never reassigned; mark it as readonly.
[warning] 883-883: Member 'handleMoveColumn' is never reassigned; mark it as readonly.
[warning] 892-892: Member 'findColumn' is never reassigned; mark it as readonly.
src/components/table/table-selection.ts
[warning] 184-184: Member 'getActiveRows' is never reassigned; mark it as readonly.
🔇 Additional comments (30)
package.json (1)
54-54: LGTM! Major version upgrade to Tabulator 6.The dependency upgrades from v4 to v6 are correctly reflected for both the library and its type definitions.
Also applies to: 93-93
src/components/table/partial-styles/tabulator-arrow.scss (3)
3-12: LGTM! Nesting updated for Tabulator v6 DOM structure.The updated selector nesting correctly targets the sorter element within Tabulator v6's new DOM hierarchy.
14-47: LGTM! Cleaner opacity control via ARIA attributes.The refactored opacity handling using
aria-sortattributes provides better semantic structure and separates sorted/unsorted states cleanly.
49-94: LGTM! Centralized arrow rotation.Moving the
rotate: 180degto the base.tabulator-arrowselector and removing per-state rotations simplifies the transform logic while maintaining the correct sorting direction convention.src/components/table/partial-styles/tabulator-custom-styles.scss (4)
16-37: LGTM! Improved transition control with CSS variables.Replacing hard-coded timing values with CSS variables (
--limel-clickable-transition-speed,--limel-clickable-transform-speed,--limel-clickable-transform-timing-function) provides better consistency and allows for easier customization.
69-69: LGTM! Hover background color added.The hover state now correctly applies the background color using the
--table-row-background-color--hovervariable.
104-121: LGTM! Refined active state feedback.The changes improve the active row interaction:
- Reduced transform (0.08rem → 0.05rem) for more subtle press feedback
- New active state styling with opacity, background color, and inset shadow provides clear visual feedback for selected rows
191-191: LGTM! Selector renamed for Tabulator v6.The class name update from
.tabulator-tableHolderto.tabulator-tableholderaligns with Tabulator v6's updated class naming convention.Also applies to: 207-207
src/components/table/partial-styles/_table-main-layout.scss (1)
19-19: LGTM! Selector renamed for Tabulator v6.Consistent with other styling files, the class name is updated to
.tabulator-tableholderto match Tabulator v6's class naming.src/components/table/table.spec.ts (1)
14-14: LGTM! Test setup updated for Tabulator v6 initialization.Setting
initialized = truein the test setup correctly reflects the new initialization flow introduced with Tabulator v6, where the component checks this flag before executing tabulator methods.src/components/table/table.scss (3)
27-27: LGTM! Active row styling updated to use primary theme color.Using
--mdc-theme-primaryfor the active row background provides better visual prominence and theme consistency compared to the previous--contrast-300.
55-55: LGTM! Selector renamed for Tabulator v6.Consistent with other files, the class name is updated to
.tabulator-tableholderto match Tabulator v6's naming convention.
75-75: LGTM! More specific hover selector.Adding
.tabulator-col-sorter-elementto the hover selector ensures the background color change only applies to columns with sorting enabled, preventing unintended hover effects on non-sortable columns.src/components/table/table-selection.spec.ts (1)
1-1: LGTM! Updated to use Tabulator v6's explicit type exports.The changes correctly migrate from namespace-based types (
Tabulator.CellComponent,Tabulator.RowComponent) to explicit named imports (CellComponent,RowComponent), aligning with Tabulator v6's updated type definitions.Also applies to: 45-72
src/components/table/examples/table-pagination.tsx (1)
1-76: LGTM! Well-documented example for the new pagination feature.The example correctly demonstrates the new
paginationLocationprop with:
- Clear JSDoc documentation
- Interactive controls to toggle pagination location
- Proper imports from
@limetech/lime-elements(as required for example files)- Shadow DOM correctly enabled
As per coding guidelines, new props should include documentation examples, and this file fulfills that requirement perfectly.
src/components/table/table.tsx (8)
11-24: LGTM! Explicit type imports align with Tabulator v6 best practices.The migration from namespace imports to explicit named imports is the correct approach for Tabulator v6 and improves tree-shaking potential.
191-197: LGTM! Well-documented new property.The
paginationLocationprop is properly documented with JSDoc, has appropriate type constraints, and includes a sensible default. The example componentlimel-example-table-paginationis referenced in the component's example list.
240-240: LGTM! Initialization flag properly tracks Tabulator readiness.The
initializedflag correctly guards against race conditions by preventing data operations until thetableBuiltevent fires.
526-543: Good refactoring to factory method with event-based initialization.The
createTabulatorfactory properly:
- Uses
TabulatorFullfor full feature access in Tabulator v6- Attaches event listeners using the
.on()method (idiomatic for v6)- Defers initial data loading until
tableBuiltfires, preventing race conditionsThe hardcoded
'https://localhost'on line 536 is intentional and safe—the comment on lines 654-656 explains this URL is never actually used sinceajaxRequestFuncintercepts all requests.
663-666: Acceptable fallback logic with optional chaining.The optional chaining (
?.) prevents null dereference. Using||instead of??forgetPage()is reasonable since you want to fallback toFIRST_PAGEfor any falsy value (including0orfalse), not justnull/undefined.
700-711: Correct migration to Tabulator v6 pagination API.The pagination configuration correctly uses the v6 pattern with
pagination: trueand separatepaginationModeproperty, replacing the v4 approach where the mode was passed directly topagination.
778-795: LGTM! Proper event handling with defaultPrevented guard.The
event.defaultPreventedcheck at lines 784-786 correctly allows child components (like row selection checkboxes) to prevent row activation by callingpreventDefault(). This addresses the previously reported issue where checking selection boxes would also open the sidepanel.
901-936: LGTM! Clean render logic with proper totalRows fallback.The
totalRows ?? this.data.lengthfallback ensures pagination calculations work whentotalRowsisn't explicitly provided. Thehas-pagination-on-topclass correctly reflects the newpaginationLocationprop.src/components/table/table-selection.ts (2)
2-7: LGTM! Consistent type import pattern.The explicit type imports with
Tabulatorprefix aliases (TabulatorColumnDefinition,TabulatorCellComponent,TabulatorRowComponent) maintain consistency with the maintable.tsxfile and clearly indicate these are Tabulator-specific types.
143-145: Good defensive guard for invalid row position.In Tabulator v6,
getPosition(true)can returnfalseif the row's position cannot be determined. This guard prevents attempting selection operations on invalid row positions.src/components/table/columns.ts (5)
2-9: LGTM! Clean explicit type imports.The explicit imports from
tabulator-tablescorrectly use the v6 type names. Unliketable.tsxandtable-selection.ts, this file doesn't alias the types (e.g.,CellComponentinstead ofTabulatorCellComponent), which is acceptable since this file deals primarily with Tabulator internals.
25-48: LGTM! Factory method correctly typed for Tabulator v6.The
createmethod now returns the explicitColumnDefinitiontype, maintaining type safety with the new import structure.
92-112: LGTM! Formatter types correctly aligned with Tabulator v6.The
Formatterreturn type andCellComponentparameter types correctly match the Tabulator v6 API expectations.
270-280: LGTM! Using Tabulator's native sorter type.Switching from a custom
TabulatorSorterinterface to the library-providedSorterFromTabletype improves maintainability and ensures compatibility with Tabulator v6's event payloads.
286-295: LGTM! Aggregator return type updated.The
ColumnCalcreturn type correctly represents Tabulator v6's aggregator options (string literals like'sum','avg', etc., or custom functions).
| if (newData.length > 0) { | ||
| this.tabulator.updateOrAddData(newData); | ||
| } |
There was a problem hiding this comment.
If there are no rows to update or add, why call updateOrAddData at all?
This avoids unnecessary work, and fixes failing e2e tests in this PR
There was a problem hiding this comment.
But this checks that there IS data 😄
There was a problem hiding this comment.
Oh, maybe you added it 😄 I'm confused
There was a problem hiding this comment.
Yes. sorry I squashed the change. so you don't see what was before. There was no if
before:
// if (newData.length > 0) {
this.tabulator.updateOrAddData(newData);
// }The intelligent said:
The e2e tests were failing because of a change in behavior in the newer version of Tabulator. The issue was:
When
updateDatawas called with empty data or identical data, the code would fall through to callingthis.tabulator.updateOrAddData(newData)
In the new Tabulator version, callingupdateOrAddData([])with an empty array triggers a "No data provided" warning/error
This caused the tests to fail with "Protocol error: Target closed" errors
The fix: Added a check before callingupdateOrAddDatato ensure we only call it when there's actually data to add
Kiarokh
force-pushed
the
upgrade-tabulator
branch
from
8b9e7e7 to
44690ad
Compare
Kiarokh
added
3rd-party
Kiarokh
force-pushed
the
upgrade-tabulator
branch
from
59d7f9c to
b8e6afe
Compare
|
@LucyChyzhova seem to have some bugs when testing the the web client. Let's wait until we can verify. Will do it tomorrow |
Kiarokh
force-pushed
the
upgrade-tabulator
branch
from
b8e6afe to
6232240
Compare
| this.destroyed = true; | ||
| this.initialized = false; | ||
|
|
||
| if (this.resizeObserver) { | ||
| this.resizeObserver.disconnect(); | ||
| this.resizeObserver = null; | ||
| } | ||
|
|
||
| if (this.tabulator) { | ||
| this.tabulator.destroy(); | ||
| this.tabulator = null; | ||
| } | ||
|
|
There was a problem hiding this comment.
when testing in the web client, an error happened after switching between Table view > List view (or any other view) > and then back to Table view.
In the console:
table.tsx:562 Data Load Error: undefined
(anonymous) @ tabulator_esm.mjs:27386
Promise.catch
load @ tabulator_esm.mjs:27385
setData @ tabulator_esm.mjs:29087
(anonymous) @ table.tsx:562
(anonymous) @ tabulator_esm.mjs:27534
_dispatch @ tabulator_esm.mjs:27533
(anonymous) @ tabulator_esm.mjs:28860
Promise.finally
_create @ tabulator_esm.mjs:28858
(anonymous) @ tabulator_esm.mjs:28770
setTimeout
Tabulator @ tabulator_esm.mjs:28769
TabulatorFull @ tabulator_esm.mjs:29616
createTabulator @ table.tsx:549
(anonymous) @ table.tsx:539
requestAnimationFrame
(anonymous) @ table.tsx:534Why did it happen according to AI, and what this fixup does:
The issue was that in the new version of Tabulator, explicitly calling setData(url) was causing a "Data Load Error" when the request was aborted (via ajaxRequesting returning false) or when requestData returned Promise.reject().
The fix: In remote mode, we no longer need to manually call setData because:
Tabulator automatically loads data when ajaxURL is set in the options
The ajaxRequestFunc callback (requestData) handles the actual data retrieval
The ajaxRequesting callback can abort the first request if data is already provided as a prop
By removing the explicit setData call, we let Tabulator's built-in initialization handle the data loading, which avoids the error.
There was a problem hiding this comment.
@LucyChyzhova has also tested this in the web client. We don't find any other issues, than the one which is solved now
You can now render the pagination component on top or at the bottom of the table, using a prop. This was previously an undocumented feature, applicable using a helper class of `has-pagination-on-top`. But now it is more accessible and discoverable feature for the consumer.
Kiarokh
force-pushed
the
upgrade-tabulator
branch
from
2a2642f to
3291b16
Compare
Summary by CodeRabbit
Release Notes
New Features
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
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: