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

Remove outline module from default modules in Viewer #2253

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

misiekhardcore
Copy link

@misiekhardcore misiekhardcore commented Oct 30, 2024

relates to #2135

Proposed Changes

We don't want to include outline module by default in Viewer modules to not include any visueal, non core functionalities

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Oct 30, 2024
@nikku
Copy link
Member

nikku commented Oct 30, 2024

Consider whether or not to add a test for this.

@misiekhardcore misiekhardcore force-pushed the 2135-multi-selection-in-viewer-causes-black-boxes branch from 0a122d2 to feb99a4 Compare October 30, 2024 16:23
@misiekhardcore misiekhardcore requested review from nikku and removed request for nikku October 30, 2024 16:28
@misiekhardcore misiekhardcore force-pushed the 2135-multi-selection-in-viewer-causes-black-boxes branch from feb99a4 to 6c63f5a Compare October 30, 2024 16:29
@misiekhardcore
Copy link
Author

misiekhardcore commented Oct 31, 2024

I would love to add a test for this. I just don't know whats the proper place for it
I added one, I think it is failing because Select still depends on it (the changes to diagram-js are not done yet, but locally with linked local diagram-js it was working.
I'm not sure what is the right way of going with this? We need to merge the changes to diagram-js first but they would still have to be released so we can bump the version in this repo?

@misiekhardcore misiekhardcore force-pushed the 2135-multi-selection-in-viewer-causes-black-boxes branch 2 times, most recently from f3180b3 to 8ad87fe Compare October 31, 2024 10:49
@nikku
Copy link
Member

nikku commented Oct 31, 2024

I'm not sure what is the right way of going with this? We need to merge the changes to diagram-js first but they would still have to be released so we can bump the version in this repo?

For changes that require upstream fix it is fine to prepare + test locally (linked) if things work. We release our libraries continuously, so the next step is to get things merged in diagram-js, release them, and incorporate them into this PR.

var viewer = new Viewer({ container: container });

// when
var outline = viewer.get('outline',false);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var outline = viewer.get('outline',false);
var outline = viewer.get('outline', false);

Copy link
Member

Choose a reason for hiding this comment

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

Good test, clearly managing the expectations!

@misiekhardcore
Copy link
Author

to make sure I understood correctly - we keep this PR open, fix diagram-js and then update this PR with new diagram-js version so it passes CI?

@nikku
Copy link
Member

nikku commented Oct 31, 2024

[...] we keep this PR open, fix diagram-js and then update this PR with new diagram-js version so it passes CI?

Exactly.

@misiekhardcore misiekhardcore force-pushed the 2135-multi-selection-in-viewer-causes-black-boxes branch from fba3a73 to ad7ef31 Compare October 31, 2024 13:16
@misiekhardcore misiekhardcore force-pushed the 2135-multi-selection-in-viewer-causes-black-boxes branch from ad7ef31 to 07bc2db Compare November 1, 2024 13:20
@misiekhardcore misiekhardcore changed the title feat(viewer): remove outline module from defalt modules list Remove outline module from default modules in Viewer Nov 1, 2024
@misiekhardcore misiekhardcore force-pushed the 2135-multi-selection-in-viewer-causes-black-boxes branch from 07bc2db to f7f882e Compare November 1, 2024 13:57
@misiekhardcore
Copy link
Author

This is now cleaned up and waits for diagram-js version bump to incorporate changes done in bpmn-io/diagram-js#943

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Currently worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants