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

Fix re. cxt events (& context menu extension) #3263

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

maxkfranz
Copy link
Member

@maxkfranz maxkfranz commented Jul 31, 2024

Cross-references to related issues. If there is no existing issue that describes your bug or feature request, then create an issue before making your pull request.

Associated issues:

Notes re. the content of the pull request. Give context to reviewers or serve as a general record of the changes made. Add a screenshot or video to demonstrate your new feature, if possible.

Checklist

Author:

  • The proper base branch has been selected. New features go on unstable. Bug-fix patches can go on either unstable or master.
  • N/A Added new CDN - PageCDN #2464 Automated tests have been included in this pull request, if possible, for the new feature(s) or bug fix. Check this box if tests are not pragmatically possible (e.g. rendering features could include screenshots or videos instead of automated tests).
  • The associated GitHub issues are included (above).
  • Notes have been included (above).

Reviewers:

  • All automated checks are passing (green check next to latest commit).
  • At least one reviewer has signed off on the pull request.
  • For bug fixes: Just after this pull request is merged, it should be applied to both the master branch and the unstable branch. Normally, this just requires cherry-picking the corresponding merge commit from master to unstable -- or vice versa.

Ref: 3.30.1 breaks cytoscape.js-context-menus #3262
@maxkfranz
Copy link
Member Author

@KonradHoeffner, would you test this on your app? You'll have to build the lib and use that build in place of what npm normally provides.

@maxkfranz
Copy link
Member Author

@hasanbalci, would you verify that this works with your extension?

@maxkfranz
Copy link
Member Author

@chrtannus, would you verify that this doesn't cause a regression re. #3005, which you reviewed before? The debug page (npm run watch) is fine for testing this.

@hasanbalci
Copy link
Contributor

@maxkfranz I verify that it now works with the context-menus extension.

@KonradHoeffner
Copy link

KonradHoeffner commented Aug 1, 2024

@KonradHoeffner, would you test this on your app? You'll have to build the lib and use that build in place of what npm normally provides.

  • @maxkfranz: Thanks for the fix, it does work in my app!

To be clear, it is a vite app and I did the following:

  1. clone this repo and switched to fix/cxt-events-re-exts
  2. npm install
  3. npm run build
  4. go to my app folder, update cytoscape.js to 3.30.1 and confirm that the context menu is still broken there
  5. replace node_modules/cytoscape/dist in my app with the dist folder of cytoscape as produced in step 3
  • vite dev --force dev mode context menu is restored after forcing vite to rebundle the dependencies
  • delete dist folder, vite build, vite preview context menu is restored in preview of the actual app build as well

@maxkfranz
Copy link
Member Author

Great. Thanks for the feedback, @hasanbalci and @KonradHoeffner.

Copy link
Member

@chrtannus chrtannus left a comment

Choose a reason for hiding this comment

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

@maxkfranz, It does not cause a regression re. #3005 -- on the debug page, pressing or clicking the right button did not interrupt any of these actions: panning, dragging a node, drag-selecting nodes with the selection rectangle (holding shift).

@maxkfranz maxkfranz merged commit 32f5786 into unstable Aug 7, 2024
2 checks passed
maxkfranz added a commit that referenced this pull request Aug 7, 2024
…t-events-re-exts

Fix re. cxt events (& context menu extension)
@maxkfranz maxkfranz mentioned this pull request Aug 7, 2024
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.

4 participants