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 and adapt indicator box component from workspace navigator #20

Merged
merged 2 commits into from
Sep 24, 2018

Conversation

jjelschen
Copy link

This doesn't do anything yet, its just the indicator box component and dependencies migrated and adapted over to this module.

I did make one significant change in that the markers are now stored with each node directly, as opposed to keeping them in a central place (was: the Workspace class), in a map with path strings as keys to look them up. While this has cleared up the code that makes up this PR, it will mean that the way marker values are being set needs to change.

One option would be to traverse the whole tree to find particular nodes to manipulate marker states. Alternatively, we could signal marker changes via message bus events. If both methods turn out to be too expensive, we would need a lookup map, again, which, this time around, I would propose to design as a "tree index", i.e. a map from node id to node object. Downside of that is the additional bookkeeping required to keep the tree structure and its index in sync…

@jjelschen
Copy link
Author

Had to turn off source maps for testing, as we have done in other projects, as well, to workaround the Angular CLI bug. However, that bug usually only caused a wrong (generic) error message in failing tests. But in this case, two unit tests failed with the message indicative of said bug, but actually ran green with sourcemaps disabled. So I'm not sure what to make of that…

@gunther-bachmann gunther-bachmann merged commit 59f6b2a into master Sep 24, 2018
@gunther-bachmann gunther-bachmann deleted the feature/indicator-boxes branch September 24, 2018 11:53
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