-
Notifications
You must be signed in to change notification settings - Fork 18
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
New: Add state setters with options #95
Changes from 4 commits
9735ce6
a252f3b
f94769a
1022aa9
a5c60a1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -378,6 +378,27 @@ export class Graph<N extends INodeBase, E extends IEdgeBase> extends Subject imp | |
} | ||
|
||
update(data?: IObserverDataPayload): void { | ||
if (data && 'type' in data && 'options' in data && 'isSingle' in data.options) { | ||
if (data.type === 'node' && data.options.isSingle) { | ||
Comment on lines
+384
to
+385
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you maybe simplify this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it could be possible, but only after reworking |
||
const nodes = this._nodes.getAll(); | ||
|
||
for (let i = 0; i < nodes.length; i++) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like these iterations through all nodes/edges can be slow. Any ideas? And do we want to clear the state of all nodes/edges or only the adjusting ones? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about using hash maps? Obviously, this would require a lot of code change. Just starting a discussion, I don't think it's doable right now. For the second question, I would say only the ones that are being adjusted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or maybe to supplement this nodes array we can keep an index where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you suggesting storing nodes in a hashmap? As far as I know, retrieving elements from an array by a known index is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If all nodes are in state X, and you want to clear them up if they are not node with ID 1, you still need to to But, you can do this and increase the memory footprint: I am not sure if it makes sense to go into this right now, but this would be the plan:
This way, if you have I would keep this as is and fix only if we see that it hurts performance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I misunderstood the functionality. I saw |
||
if (nodes[i].id !== data.id) { | ||
nodes[i].clearState(); | ||
} | ||
} | ||
} | ||
|
||
if (data.type === 'edge' && data.options.isSingle) { | ||
const edges = this._edges.getAll(); | ||
|
||
for (let i = 0; i < edges.length; i++) { | ||
if (edges[i].id !== data.id) { | ||
edges[i].clearState(); | ||
} | ||
} | ||
} | ||
} | ||
this.notifyListeners(data); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,24 @@ | ||
import { GraphObject } from '../utils/observer.utils'; | ||
|
||
// Enum is dismissed so user can define custom additional events (numbers) | ||
export const GraphObjectState = { | ||
NONE: 0, | ||
SELECTED: 1, | ||
HOVERED: 2, | ||
}; | ||
|
||
export interface IGraphObjectStateOptions { | ||
isToggle: boolean; | ||
isSingle: boolean; | ||
} | ||
|
||
export interface IGraphObjectStateParameters { | ||
state: number; | ||
options?: Partial<IGraphObjectStateOptions>; | ||
} | ||
|
||
export interface ISetStateDataPayload { | ||
id: any; | ||
type: GraphObject; | ||
options: Partial<IGraphObjectStateOptions>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is nitpicking really, but maybe
getNextState
would be a better name.