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

Update to latest client changes #62

Merged
merged 2 commits into from
Jun 21, 2024
Merged

Update to latest client changes #62

merged 2 commits into from
Jun 21, 2024

Conversation

tortmayr
Copy link
Contributor

What it does

  • Update glsp next dependencies
  • Use debuggable feature ids for all vscode feature modules
  • Fix bug with vscodeNavigationModule that accidently reused the feature id from the navigation module instead of requiring it
  • Adjust tool palette css
  • Customize css for newly introduced features
  • Ensure that GLSPProjectionView is properly rendered
    • disable overflow of root div
    • Add mouse listeners to diagram widget to only show projections bars when the diagram is focused
  • Update example1.wf
  • Fix title menu contributions. Vscode now requires an icon to render them properly

How to test

Follow-ups

Changelog

  • This PR should be mentioned in the changelog
  • This PR introduces a breaking change (if yes, provide more details below for the changelog and the migration guide)

@tortmayr tortmayr requested a review from martin-fleck-at June 20, 2024 15:32
- Update glsp next dependencies
- Use debuggable feature ids for all vscode feature modules
- Fix bug with `vscodeNavigationModule` that accidently reused the feature id from the navigation module instead of requiring it
- Adjust tool palette css
- Customize css for newly introduced features
- Ensure that `GLSPProjectionView` is properly rendered
   - disable overflow of root div
   - Add mouse listeners to diagram widget to only show projections bars when the diagram is focused
- Update example1.wf
- Fix title menu contributions. Vscode now requires an icon to render them properly
Copy link
Contributor

@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.

Change looks good to me and works well besides that I have some issues with the navigation and showing of the documentation. Maybe it is something on my side (see inline comment). As soon as I figure out what that could be, this can be merged. Thank you for the good work, Tobias!

example/workflow/extension/package.json Show resolved Hide resolved
- Fix "goToPreviousNode" command definition
- Ensure that diagram widget focus state changes are forwarded to the GLSP FocusTracker
@tortmayr
Copy link
Contributor Author

Thanks for the review

The 'go to previous' is never even shown.

Fixed

The 'go to right' gets stuck quickly but that might be the expected behavior?
I guess you were testing on a fork or decision node. Then this is expected behavior, has the previous/next nav commands only work for single connected edges

The 'Show documentation...' does not work even if I have the 'Push' node selected.

Nice catch. Should be fixed with eclipse-glsp/glsp-server-node#89

Copy link
Contributor

@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.

I can confirm that it works with the newer server version! Also the navigation now shows up properly. One minor thing that could be improved is the order of the two buttons, it is a bit counter-intuitive:
image

But that should not stop this PR from being merged.

@tortmayr
Copy link
Contributor Author

tortmayr commented Jun 21, 2024

One minor thing that could be improved is the order of the two buttons, it is a bit counter-intuitive:

Agreed, but I already wasted an unreasonable amount of time trying to fix that.
The problem is that the normal approach of ordering menu contributions (group:navigation@1 etc) does not seem to work for the editor/title menu.
The order in the package.json also seems to have no effect (previous command is before the next but still rendered after)

I guess the only way to fix this would to change the command names to enforce correct lexicographical ordering. But that would be a breaking change which seems a bit overkill for that.

@martin-fleck-at
Copy link
Contributor

Interesting, yeah, then let's leave it as it is for now. Thank you!

@tortmayr tortmayr merged commit 0b7b0dd into master Jun 21, 2024
3 checks passed
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