fix: update zIndex while calling setParent & deduplicate while translating. #6295

HawtinZeng · 2024-09-07T11:02:03Z

fix(DataController): after calling graph.addChildrenData(nodeInCombo, [nodeNotInCombo]), can not listen click event of added node.
fix(DataController): translateComboBy will make twice tranlations to the same child node of a combo

HawtinZeng · 2024-09-07T11:16:07Z

Hi, antv teams, there are two flaws I have found, watch the gif below:
screen-capture.webm
screen-capture (1).webm

We need to change the zIndex of child node into the parent node intuitively, when we change to add a node as child of another node, but when we want to move a child node into a combo, we need make nodo zIndex bigger than the combo zIndex.
When when we have two collected nodes included by a combo, the child node will be translated twice in translateComboBy.
Hope this pr can be helpful.

Aarebecca · 2024-09-09T01:59:19Z

Thanks for your contribution, we will confirm and validate this pull request soon.

Aarebecca · 2024-09-09T02:07:26Z

packages/g6/src/runtime/data.ts

+          eleData.style = {};
+        }
+        if (this.isCombo(parent)) {
+          eleData.style.zIndex = parentData.style!.zIndex + 1;


It's better to add an alternative value when parentData.style.zIndex is undefined, and you can use set and get function which exported from @antv/util to simplify the code,
like:

const zIndex = get(parentData, ['style', 'zIndex'], 0) + this.isCombo(parent) ? 1 : 0; set(elementData, ['style', 'zIndex'], zIndex);

Glad to receive your review. We shuldn't change the original action about zIndex, because many(10+ test suits) depend on the original zIndex version. Besides, thanks for your concise code suggestion, we should wrap the ternary with ().

packages/g6/src/runtime/data.ts

Aarebecca · 2024-09-09T02:49:32Z

It seems that the test snapshots should be updated, you can re-generate related snapshots as follows:

remove current snapshots:

rm -rf packages/g6/__tests__/snapshots/behaviors/drag-element-combo
rm -rf packages/g6/__tests__/snapshots/behaviors/collapse-expand-node
rm -rf packages/g6/__tests__/snapshots/plugins/history/plugin-history

re-generate snapshots:

cd ./packages/g6
npm run test

submit all new snapshots files

HawtinZeng · 2024-09-09T16:02:51Z

All tests passed, maybe the test suit should be less strict.

HawtinZeng · 2024-09-09T16:09:30Z

By the way, these svg changes are messy : (

Aarebecca · 2024-09-10T06:12:38Z

By the way, these svg changes are messy : (

Generally speaking, a version of the modification will not cause such a huge change in the screenshots. I can't even view the changes on GitHub. I switched to your branch to view yesterday. In fact, only a dozen screenshots need to be updated.

Aarebecca · 2024-09-10T06:16:54Z

@HawtinZeng The screenshots you updated are located in /__tests__/snapshots/__tests__, which is abnormal. Did you execute the npm run test command under ./packages/g6?

Aarebecca · 2024-09-11T09:01:59Z

Hi @HawtinZeng , I believe this bug fix is important and should be merged as soon as possible. Here's the pull request: #6320. You can refer to this PR for modifications. If you don’t have time to review it soon, we will close this PR and merge an alternative one.

HawtinZeng · 2024-09-11T09:33:06Z

I will commit some changes tonight.

…

---Original--- From: ***@***.***> Date: Wed, Sep 11, 2024 17:02 PM To: ***@***.***>; Cc: "Hawtin ***@***.******@***.***>; Subject: Re: [antvis/G6] fix: update zIndex while calling setParent &deduplicate while translating. (PR #6295) Hi @HawtinZeng , I believe this bug fix is important and should be merged as soon as possible. Here's the pull request: #6320. You can refer to this PR for modifications. If you don’t have time to review it soon, we will close this PR and merge an alternative one. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

HawtinZeng · 2024-09-11T15:51:00Z

Hi, Aaron, I have tried many ways to pass jest, but failed finally, there are 3 units tests haven't pass, I can pass these tests with running a single test, but can't pass the test when running all tests, maybe some contexts have interference to these failed tests.

Aarebecca · 2024-09-12T02:41:40Z

@HawtinZeng You can refer to the methods I mentioned earlier. Normal operations should not lead to the problems you encountered. Be sure to note that when you execute the npm run test command, the current terminal path should be under packages/g6. Also, packages/g6/tests/utils/dir.ts is a stable code and it should not be modified.

A simple test review:

Remove all snapshots generated by this pull request
Remove the current G6 snapshots and it should be updated because related code has been modified.
Re-generate snapshots baseline for this pull request

Aarebecca · 2024-09-12T02:47:39Z

The pull request: #6320 is a standard submit, and your modification scope should not exceed it. (Ignore the coverage)

HawtinZeng · 2024-09-12T03:35:37Z

OK

…

---Original--- From: ***@***.***> Date: Thu, Sep 12, 2024 10:48 AM To: ***@***.***>; Cc: "Hawtin ***@***.******@***.***>; Subject: Re: [antvis/G6] fix: update zIndex while calling setParent &deduplicate while translating. (PR #6295) The pull request: #6320 is a standard submit, and your modification scope should not exceed it. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

HawtinZeng · 2024-09-12T15:32:34Z

packages/g6/__tests__/unit/plugins/history/plugin-history.spec.ts

I think the original test suit is wrong, so I change the zIndex of node-1 to avoid subsequent test errors.

HawtinZeng · 2024-09-12T15:34:23Z

packages/g6/__tests__/utils/dir.ts

In Windows, the cwd path is: 'xxx\\xxx\\xxx.xxx', we need path.sep to fix this.

Aarebecca · 2024-09-13T02:35:44Z

@HawtinZeng The latest commit looks much better, thanks for your efforts, and the final thing is to remove the files under ./packages/g6/__tests__/snapshots/__tests__.
It was an oversight, I didn't realize we were using different operating systems, sorry for the trouble this has caused you.

* fix: deduplicate multiple translations to the same node * test: update zIndex related snapshots * fix: deal with path seperator in Windows

HawtinZeng · 2024-09-13T13:51:32Z

Hi, Aarebecca, finished this PR finally.

* fix: change zIndex in DataController (#6295) * fix: deduplicate multiple translations to the same node * test: update zIndex related snapshots * fix: deal with path seperator in Windows * test: update test snapshots --------- Co-authored-by: Hawtin Zeng <73981739+HawtinZeng@users.noreply.github.com> Co-authored-by: antv <antv@antfin.com>

Aarebecca reviewed Sep 9, 2024

View reviewed changes

HawtinZeng force-pushed the v5 branch 3 times, most recently from 0a83370 to 36b9d63 Compare September 9, 2024 16:01

Aarebecca mentioned this pull request Sep 11, 2024

fix: update zIndex while calling setParent & deduplicate while translating. #6320

Closed

HawtinZeng force-pushed the v5 branch from 53b3ce1 to 2b9f5f9 Compare September 11, 2024 16:11

HawtinZeng force-pushed the v5 branch from 2b9f5f9 to 78d4c73 Compare September 12, 2024 15:28

HawtinZeng commented Sep 12, 2024

View reviewed changes

fix: change zIndex in DataController

ba69902

* fix: deduplicate multiple translations to the same node * test: update zIndex related snapshots * fix: deal with path seperator in Windows

HawtinZeng force-pushed the v5 branch from 78d4c73 to ba69902 Compare September 13, 2024 13:48

Aarebecca changed the base branch from v5 to fix/z-index September 14, 2024 06:38

Aarebecca approved these changes Sep 14, 2024

View reviewed changes

Aarebecca merged commit 31f296b into antvis:fix/z-index Sep 14, 2024
1 of 2 checks passed

nerds-github mentioned this pull request Oct 28, 2024

[Snyk] Upgrade @antv/g6 from 4.8.24 to 5.0.25 nerds-github/ant-design#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update zIndex while calling setParent & deduplicate while translating. #6295

fix: update zIndex while calling setParent & deduplicate while translating. #6295

HawtinZeng commented Sep 7, 2024

HawtinZeng commented Sep 7, 2024 •

edited

Loading

Aarebecca commented Sep 9, 2024

Aarebecca Sep 9, 2024

HawtinZeng Sep 9, 2024 •

edited

Loading

Aarebecca commented Sep 9, 2024

HawtinZeng commented Sep 9, 2024

HawtinZeng commented Sep 9, 2024

Aarebecca commented Sep 10, 2024

Aarebecca commented Sep 10, 2024 •

edited

Loading

Aarebecca commented Sep 11, 2024

HawtinZeng commented Sep 11, 2024 via email

HawtinZeng commented Sep 11, 2024 •

edited

Loading

Aarebecca commented Sep 12, 2024 •

edited

Loading

Aarebecca commented Sep 12, 2024 •

edited

Loading

HawtinZeng commented Sep 12, 2024 via email

HawtinZeng Sep 12, 2024

HawtinZeng Sep 12, 2024 •

edited

Loading

Aarebecca commented Sep 13, 2024 •

edited

Loading

HawtinZeng commented Sep 13, 2024

fix: update zIndex while calling setParent & deduplicate while translating. #6295

fix: update zIndex while calling setParent & deduplicate while translating. #6295

Conversation

HawtinZeng commented Sep 7, 2024

HawtinZeng commented Sep 7, 2024 • edited Loading

Aarebecca commented Sep 9, 2024

Aarebecca Sep 9, 2024

Choose a reason for hiding this comment

HawtinZeng Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Aarebecca commented Sep 9, 2024

HawtinZeng commented Sep 9, 2024

HawtinZeng commented Sep 9, 2024

Aarebecca commented Sep 10, 2024

Aarebecca commented Sep 10, 2024 • edited Loading

Aarebecca commented Sep 11, 2024

HawtinZeng commented Sep 11, 2024 via email

HawtinZeng commented Sep 11, 2024 • edited Loading

Aarebecca commented Sep 12, 2024 • edited Loading

Aarebecca commented Sep 12, 2024 • edited Loading

HawtinZeng commented Sep 12, 2024 via email

HawtinZeng Sep 12, 2024

Choose a reason for hiding this comment

HawtinZeng Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Aarebecca commented Sep 13, 2024 • edited Loading

HawtinZeng commented Sep 13, 2024

HawtinZeng commented Sep 7, 2024 •

edited

Loading

HawtinZeng Sep 9, 2024 •

edited

Loading

Aarebecca commented Sep 10, 2024 •

edited

Loading

HawtinZeng commented Sep 11, 2024 •

edited

Loading

Aarebecca commented Sep 12, 2024 •

edited

Loading

Aarebecca commented Sep 12, 2024 •

edited

Loading

HawtinZeng Sep 12, 2024 •

edited

Loading

Aarebecca commented Sep 13, 2024 •

edited

Loading