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

Feature/editable graph view #22

Merged
merged 44 commits into from
Sep 4, 2023
Merged

Conversation

ozcanxbreeze
Copy link
Contributor

@ozcanxbreeze ozcanxbreeze commented Jul 5, 2023

A pull request for the following tasks:
- Add editable labels to the graphical view
- Add "add attribute" action to the graphical view
- Add delete attribute action to the graphical view
- Make changing of attributes order possible in graphical view

Animatie

check for documentation

Also did the following to get it working:
- Implemented a property view
- Moved the Form-server and Form-client to a seperate package, so the property view can also make use of it

Made new tasks for the following todo's:
- There are a couple of react views reuesed, These should be moved to a single package and imported.
- The glsp view does not get updated, when the property view changes the node.
- Error handling for the row processing

@Moo925 Moo925 self-requested a review July 11, 2023 07:25
@harmen-xb harmen-xb self-requested a review July 14, 2023 13:18
@martin-fleck-at martin-fleck-at self-requested a review August 29, 2023 12:20
Copy link
Collaborator

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ozcanxbreeze, great work! I had a look at the code and tested the functionality: Adding, moving and deleting attributes works but I'm not sure about the editable labels. Overall the change works well but I do have some questions and suggestions:

  • Deselecting an element does not clear the property view.
  • Adding attributes and hitting save does not update the GLSP diagram - not even when closing and re-opening. Only after opening the affected entity file, the diagram loads the data correctly.
  • In the 'Data type' field of the properties view, I can type custom text, is that expected?
  • Name changes in the form break cross references since they are name-based. Probably we require a rename refactoring here. I don't suggest to do this as part of this PR but maybe we should create a ticket for that or make the field in the form read-only.
  • The tickets you mentioned are talking about changes in the "graphical" view. Is this covered by the properties view or are you planning any in-line diagram changes?

@martin-fleck-at
Copy link
Collaborator

@ozcanxbreeze I see that you pushed commit 693e8fc just now. I did not consider this in my review as I started before that. So please disregard any comments that are outdated because of it.

@ozcanxbreeze
Copy link
Contributor Author

Hey Martin, Thanks for the review. I indeed did push some changes, because I wanted to switch branches to review your pull request. I did not manage to get it functionally working yet, so https://dev.azure.com/x-breeze/CrossModel/_workitems/edit/834 is still open.

I also just merged your branch into main, so I will have to do some merging and refactoring to get it working again.

@ozcanxbreeze ozcanxbreeze force-pushed the feature/editable-graph-view branch from 62bcd13 to 6d29932 Compare August 31, 2023 14:57
@martin-fleck-at martin-fleck-at force-pushed the feature/editable-graph-view branch from 6d29932 to 9087904 Compare August 31, 2023 15:11
@martin-fleck-at
Copy link
Collaborator

martin-fleck-at commented Aug 31, 2023

@ozcanxbreeze I pushed an update that should resolve the issue we faced with the broken build. Basically what caused the problem was that in the model-service package the index was put outside of the browser and node directory but it exported the files from there. So when we did an import from that index from the frontend, it automatically tried to resolve the exported node files as well and then broken with the missing fs dependency that was introduced by my commit in the meantime.

Please note that I force pushed but I kept your changes.

@ozcanxbreeze
Copy link
Contributor Author

ozcanxbreeze commented Sep 1, 2023

@martin-fleck-at Awesome, I am going to resolve the pull request review issues, and then I will notify you again

@ozcanxbreeze
Copy link
Contributor Author

ozcanxbreeze commented Sep 1, 2023

@martin-fleck-at Hey Marin, I just pushed a new commit that implement your review points and your remarks.

  • Deselecting an element does not clear the property view.
    • This works now
  • Adding attributes and hitting save does not update the GLSP diagram - not even when closing and re-opening. Only after opening the affected entity file, the diagram loads the data correctly.
    • Should also work now
      In the 'Data type' field of the properties view, I can type custom text, is that expected?
    • No :P, Should work correctly now
  • Name changes in the form break cross references since they are name-based. Probably we require a rename refactoring here. I don't suggest to do this as part of this PR but maybe we should create a ticket for that or make the field in the form read-only.
    • I made it read-only
  • The tickets you mentioned are talking about changes in the "graphical" view. Is this covered by the properties view or are you planning any in-line diagram changes?
    • We talked about doing it in the graphical view, but changed it to doing it in a separate view and concluded that the property view was the best way to implement it.

Copy link
Collaborator

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ozcanxbreeze I added a minor fix to one of the package.json but otherwise everything looks good to me now! Thank you!

@ozcanxbreeze ozcanxbreeze merged commit 54b189e into main Sep 4, 2023
@ozcanxbreeze ozcanxbreeze deleted the feature/editable-graph-view branch September 4, 2023 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants