Add expandable subcategories with lazy loading#6325
Add expandable subcategories with lazy loading#6325EugenBodanov wants to merge 17 commits intosaleor:mainfrom
Conversation
…nd targeted cache invalidation
🦋 Changeset detectedLatest commit: 21981f2 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6325 +/- ##
==========================================
+ Coverage 42.79% 42.81% +0.02%
==========================================
Files 2497 2500 +3
Lines 43405 43784 +379
Branches 10232 10286 +54
==========================================
+ Hits 18573 18745 +172
+ Misses 24795 23708 -1087
- Partials 37 1331 +1294 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the contribution. I tested locally and it looks good, left minor comments Extra idea: you may want to store tree state in local storage, so once you go back the tree is restored |
|
Thanks for the contribution @EugenBodanov! Can you replace the import { iconSize, iconStrokeWidthBySize, iconStrokeWidth } from "@dashboard/components/icons";
import { ChevronRight } from "lucide-react";
// Small icons (16px) - use thicker stroke for visibility
<ChevronRight size={iconSize.small} strokeWidth={iconStrokeWidthBySize.small} /> |
|
Hi, I replaced the text-based expand indicator with a custom chevron cell in the datagrid. |
|
@lkostrowski @mirekm Just a heads-up that the PR is ready for review with the requested updates. |
|
@EugenBodanov please check build |
|
@lkostrowski Hi, I've added a type declaration for vite.config.js, so check-types should pass now. I also merged the latest main branch, which should resolve the 'Regression detected: +3 imports' error. |
| expect(result).toBe(true); | ||
| expect(ctx.save).toHaveBeenCalledTimes(1); | ||
| expect(ctx.beginPath).toHaveBeenCalledTimes(1); | ||
| expect(ctx.moveTo).toHaveBeenCalledWith(14, 12); | ||
| expect(ctx.lineTo).toHaveBeenNthCalledWith(1, 18, 16); | ||
| expect(ctx.lineTo).toHaveBeenNthCalledWith(2, 14, 20); | ||
| expect(ctx.strokeStyle).toBe("#111111"); | ||
| expect(ctx.lineWidth).toBe(2); | ||
| expect(ctx.lineCap).toBe("round"); | ||
| expect(ctx.lineJoin).toBe("round"); | ||
| expect(ctx.stroke).toHaveBeenCalledTimes(1); | ||
| expect(ctx.restore).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
this assertion set doesnt make sense, no one will verify internals of how its drawn, you can simplify it to something stable
| const lucideChevronPoints = { | ||
| down: [ | ||
| [6, 9], | ||
| [12, 15], | ||
| [18, 9], | ||
| ], | ||
| right: [ | ||
| [9, 6], | ||
| [15, 12], | ||
| [9, 18], | ||
| ], | ||
| } as const; |
There was a problem hiding this comment.
is this trying to emulate lucide svg? Idea is fine, but maybe lets add comment what is done here
| describe("allRipples", () => { | ||
| it("should include expandable subcategories ripple exactly once", () => { | ||
| // Arrange | ||
| const matchingRipples = allRipples.filter( | ||
| ripple => ripple.ID === rippleExpandedSubcategories.ID, | ||
| ); | ||
|
|
||
| // Act | ||
| const count = matchingRipples.length; | ||
|
|
||
| // Assert | ||
| expect(count).toBe(1); | ||
| expect(matchingRipples[0]).toBe(rippleExpandedSubcategories); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
i dont think we need that, you may add generic test that any ripple is at most once
There was a problem hiding this comment.
I dont see a point of having this test
Motivation Navigating through multiple levels of the category tree just to edit a single item is inefficient and inconvenient for the user. This PR introduces expandable rows, allowing users to access and edit subcategories directly from the parent list without traversing the entire hierarchy.
Key Changes