Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to version 2.x antv/x6 #1099

Merged
merged 39 commits into from
Oct 15, 2024
Merged

Migrate to version 2.x antv/x6 #1099

merged 39 commits into from
Oct 15, 2024

Conversation

jgadsden
Copy link
Collaborator

@jgadsden jgadsden commented Oct 4, 2024

Summary:
first pass at migrating to version 2.x antv/x6
unit tests broken
stencil incomplete
various unfinished features

Description for the changelog:
migrate to version 2.x antv/x6

Other info:

Closes #722

@jgadsden
Copy link
Collaborator Author

jgadsden commented Oct 4, 2024

@lreading feel free to progress this, there is a lot of work still to do
the data flows are now very easy to use so the migration will certainly be worth it

@leo-reading-springhealth

Looking at the tests right now, they're giving me a hard time. 😩

I only tested the history implementation, and that seems to work. I'll want to test all the other functions that are implemented via plugin now, as they may or may not work the same. I found that calling graph.history was returning undefined, graph.getPlugin(<name>) gets the history plugin which seems to have the same interface as the old version.

@leo-reading-springhealth

Wait, let me try again - I had my wires crossed. I'm going to start anew from this branch! I had created a new branch and copied some of the work from your old PR. This is even better, thank you @jgadsden !

@leo-reading-springhealth

Do we want to delete the trust boundary curve and flow stencils?

@jgadsden jgadsden marked this pull request as draft October 5, 2024 05:50
@jgadsden
Copy link
Collaborator Author

jgadsden commented Oct 5, 2024

Many thanks for the additions @lreading
I agree we need to have flows and TB curves in the stencils, they are just commented out for now because (I think) they are handled differently in the new stencil package - but I probably have that wrong
There is still a lot of work to do, for example scrolling the diagram produces very unexpected results :)
The blocking bug where only part of the diagram is displayed has been fixed, but reappears when the diagram is saved, and so on

@jgadsden
Copy link
Collaborator Author

jgadsden commented Oct 5, 2024

Do we want to delete the trust boundary curve and flow stencils?

definitely want to keep trust boundary curve and flow stencils, it was only that I could not see how to migrate them when I committed these changes

@lreading
Copy link
Collaborator

lreading commented Oct 6, 2024

There's a bug when clicking undo very quickly after populating the history stack. No fix for it yet, but I wanted to comment on it so that I don't forget to look into it more. :)

image

[Thanks for fixing this @lreading ]

@jgadsden
Copy link
Collaborator Author

jgadsden commented Oct 6, 2024

There's a bug when clicking undo very quickly after populating the history stack.

yes, agreed -we probably should check for state.cell in :

        disableNewThreat: function (state) {
            return state.cell.ref.data.outOfScope || state.cell.ref.data.isTrustBoundary || state.cell.ref.data.type === 'tm.Text';
        }

@jgadsden
Copy link
Collaborator Author

jgadsden commented Oct 13, 2024

The only thing left is the unit tests which are in progress and a bug where the the diagram in the report is still modifiable - freeze/unfreeze was removed from antv/x6 1.x -> 2.x and the only reference I can find for something similar in version 2.x is to do with graph.async boolean, and this does not seem to be effective

@lreading I can work on the unit tests and raise an issue for the mutable report diagram - unless you want to determine how it can be done, possibly use graph.toPNG instead ?

@lreading
Copy link
Collaborator

It looks like setting interacting: false on the readonly graph gets us really close to an immutable copy for reporting. The cursor will still change to a pointer when hovering over components, but you cannot move them or reconnect edges. I think this will be good enough? 😄

I'm going to take a peek at the unit tests as well!

@jgadsden
Copy link
Collaborator Author

wondering if we should provide more ports along the edges of the node components? We have one each side centred , which is a 'must have' , but how about 3 per side to provide less bunching?

ie something like in ports.js :

    items: [
        {
	    group: 'top'
        },
        {
	    group: 'top'
        },
        {
	     group: 'top'
        },
        {
             group: 'right'
        },

to give:

Screenshot 2024-10-14 at 18 51 25

@lreading
Copy link
Collaborator

wondering if we should provide more ports along the edges of the node components? We have one each side centred , which is a 'must have' , but how about 3 per side to provide less bunching?

ie something like in ports.js :

    items: [
        {
	    group: 'top'
        },
        {
	    group: 'top'
        },
        {
	     group: 'top'
        },
        {
             group: 'right'
        },

to give:

Screenshot 2024-10-14 at 18 51 25

I think that'd be a great addition!

@lreading
Copy link
Collaborator

The unit tests are now passing, and it looks like the e2e tests did as well! 🎉

The coverage is probably down from where it once was, I was primarily focused on getting them passing. :)

@jgadsden
Copy link
Collaborator Author

thanks @lreading for all this - we are ready to commit it I think? The honour is yours :)
maybe I can sneak in one last update to the number of ports ...

@jgadsden jgadsden marked this pull request as ready for review October 15, 2024 05:11
td.vue/package.json Outdated Show resolved Hide resolved
td.vue/src/service/x6/ports.js Outdated Show resolved Hide resolved
@lreading
Copy link
Collaborator

I'll leave it up to you @jgadsden ! If you feel it's ready for merge, by all means 🙌

@jgadsden
Copy link
Collaborator Author

apologies @lreading , I was sneaking in a last minute change which removes the ports from the text box - the thinking being that we don't want dataflows attaching to a text box
I also went against the idea of multiple ports on each edge - it did not add to the usability and looked a bit strange
please go ahead and merge, many thanks

@lreading lreading merged commit 6151de8 into main Oct 15, 2024
10 checks passed
@lreading lreading deleted the v2-antv-x6 branch October 15, 2024 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment