Add tree-control expand/collapse on click the expander button or by a custom logic #36434

mdperez86 · 2023-01-14T01:46:46Z

All Submissions:

Have you followed the WooCommerce Contributing guideline?
Does your code follow the WordPress' coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Depends on #36432
Partially addresses #35851

This PR adds expand/collapse functionality to the TreeControl component.

This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

Build storybook (pnpm --filter=@woocommerce/storybook build-storybook)
Run storybook (pnpm --filter=@woocommerce/storybook storybook)
Visit http://localhost:6007/?path=/story/woocommerce-admin-experimental-treecontrol--expand-on-search
A basic tree control should be shown.
If you click the expander button at the end of the item the subtree should expand/collapse.
If you type in the text control above the tree should expand to show the matching children.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?

FOR PR REVIEWER ONLY:

I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

github-actions · 2023-01-14T02:12:55Z

Test Results Summary

Commit SHA: 34e6d86

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	1m 15s
E2E Tests	189	0	0	6	0	195	17m 29s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

… custom logic

joshuatf

These are awesome hooks and very nicely isolated! Left a few very nitpicky naming suggestions and a suggestion on premature optimization.

joshuatf · 2023-01-30T21:51:38Z

packages/js/components/src/experimental-tree-control/stories/index.tsx

+				<TreeControl
+					id="expand-on-search"
+					items={ listItems }
+					isItemExpanded={ useCallback(


I don't think this useCallback offers any optimization and in fact may worsen performance.

The only variable coming from outside this function is text which also is a dependency of the callback, so memoization of this function increases space complexity.

I think that useCallback is useful here. Otherwise, every time the parent of TreeControl was re-rendered, TreeControl would be re-rendered because the function passed into isItemExpanded would be re-created.

The function passed in is doing recursive checking of children items to see if an item should be expanded, so it seems potentially expensive if there is a large tree.

In this particular story, I don't think it really makes any noticeable difference though.

I could do either way, either keeping useCallback or getting rid of it for simplicity in this particular example.

Currently in this example, the only time this should be re-rendered is when text is changed, right? We would also want to re-render in that case and it does invalidate the memoization on the callback.

I think for this story (and most) the performance changes are going to be negligible either way, but since this decreases code readability and these stories act as some of the primary example for 3PDs, erring on the side of simplicity is best IMO.

Agreed. Simplicity is the way to go. I'll update this.

joshuatf · 2023-01-30T21:55:50Z

packages/js/components/src/experimental-tree-control/hooks/use-expander.ts

+
+	useEffect( () => {
+		if ( item.children?.length && typeof isExpanded === 'function' ) {
+			setExpanded( isExpanded( item ) );


To make this a bit more clear and avoid ambiguity, maybe we could update the names slightly.

expanded -> isExpanded (boolean) isExpanded -> shouldExpand (function)

joshuatf · 2023-01-30T21:58:08Z

packages/js/components/src/experimental-tree-control/stories/index.tsx

@@ -36,6 +36,36 @@ export const SimpleTree: React.FC = () => {
 	);
 };

+function isItemExpanded( item: LinkedTree, text: string ) {


Same comment here re: is/should. I also think text might be better as query if I'm understanding what this is correctly (user search input).

Suggested change

function isItemExpanded( item: LinkedTree, text: string ) {

function shouldItemExpand( item: LinkedTree, query: string ) {

mattsherman · 2023-01-31T18:58:14Z

@joshuatf I've address your suggestions and renamed the expanded-related props.

I'm on the fence regarding the use of useCallback in the story example.

joshuatf

Thanks for the discussion on this issue! Changes look good and still testing well 🎉

joshuatf

Changes look good, thanks for the update!

mdperez86 added focus: product management Related to product creation and editing. focus: new product ux revamped product management experience labels Jan 14, 2023

mdperez86 requested a review from a team January 14, 2023 01:46

mdperez86 self-assigned this Jan 14, 2023

github-actions bot added the package: @woocommerce/components issues related to @woocommerce/components label Jan 14, 2023

mdperez86 mentioned this pull request Jan 14, 2023

Add selection to the tree control #36435

Merged

8 tasks

mdperez86 marked this pull request as ready for review January 17, 2023 13:27

mdperez86 force-pushed the add/35851-tree-control-expander branch from ff04deb to 49c15b8 Compare January 17, 2023 14:08

mdperez86 mentioned this pull request Jan 17, 2023

Add custom rendering logic to the item label #36476

Merged

8 tasks

mattsherman assigned mattsherman and unassigned mdperez86 Jan 19, 2023

mattsherman force-pushed the add/35851-tree-control-expander branch from 49c15b8 to baa3cb5 Compare January 19, 2023 20:26

mattsherman mentioned this pull request Jan 24, 2023

[Epic] TreeControl component #36590

Closed

7 tasks

mattsherman added the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Jan 24, 2023

Add tree-control expand/collapse on click the expander button or by a…

e4572ac

… custom logic

mattsherman removed the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Jan 27, 2023

mattsherman force-pushed the add/35851-tree-control-expander branch from baa3cb5 to e4572ac Compare January 27, 2023 16:58

mattsherman added 3 commits January 27, 2023 12:08

Update changelog

63dc7f8

Clean up styles

e580d3e

Update isItemExpanded code doc

ae7cea7

joshuatf reviewed Jan 30, 2023

View reviewed changes

mattsherman added 3 commits January 31, 2023 13:08

Rename expanded props

ad64f7a

Destructure expander

cc71bcb

Update shouldItemBeExpanded code doc

a27a754

joshuatf previously approved these changes Feb 1, 2023

View reviewed changes

mattsherman added 2 commits January 31, 2023 21:07

Remove useCallback from story

2984fc8

Remove useCallback from shouldItemBeExpanded code doc

34e6d86

mattsherman dismissed joshuatf’s stale review via 34e6d86 February 1, 2023 02:08

joshuatf approved these changes Feb 1, 2023

View reviewed changes

mattsherman merged commit 5570f6a into trunk Feb 1, 2023

mattsherman deleted the add/35851-tree-control-expander branch February 1, 2023 17:04

github-actions bot added this to the 7.5.0 milestone Feb 1, 2023

AnnaMag unassigned mattsherman Feb 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tree-control expand/collapse on click the expander button or by a custom logic #36434

Add tree-control expand/collapse on click the expander button or by a custom logic #36434

mdperez86 commented Jan 14, 2023 •

edited by mattsherman

github-actions bot commented Jan 14, 2023 •

edited

joshuatf left a comment

joshuatf Jan 30, 2023

mattsherman Jan 31, 2023

joshuatf Jan 31, 2023

mattsherman Jan 31, 2023

joshuatf Jan 30, 2023

joshuatf Jan 30, 2023

mattsherman commented Jan 31, 2023

joshuatf left a comment

joshuatf left a comment

	function isItemExpanded( item: LinkedTree, text: string ) {
	function shouldItemExpand( item: LinkedTree, query: string ) {

Add tree-control expand/collapse on click the expander button or by a custom logic #36434

Add tree-control expand/collapse on click the expander button or by a custom logic #36434

Conversation

mdperez86 commented Jan 14, 2023 • edited by mattsherman

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

FOR PR REVIEWER ONLY:

github-actions bot commented Jan 14, 2023 • edited

Test Results Summary

joshuatf left a comment

Choose a reason for hiding this comment

joshuatf Jan 30, 2023

Choose a reason for hiding this comment

mattsherman Jan 31, 2023

Choose a reason for hiding this comment

joshuatf Jan 31, 2023

Choose a reason for hiding this comment

mattsherman Jan 31, 2023

Choose a reason for hiding this comment

joshuatf Jan 30, 2023

Choose a reason for hiding this comment

joshuatf Jan 30, 2023

Choose a reason for hiding this comment

mattsherman commented Jan 31, 2023

joshuatf left a comment

Choose a reason for hiding this comment

joshuatf left a comment

Choose a reason for hiding this comment

mdperez86 commented Jan 14, 2023 •

edited by mattsherman

github-actions bot commented Jan 14, 2023 •

edited