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

Chore: Apply Emitter in simulator and fix cleanup when terminated, Refactor naming #25

Merged
merged 14 commits into from
Oct 10, 2022

Conversation

cizl
Copy link
Contributor

@cizl cizl commented Oct 5, 2022

No description provided.

@cizl cizl self-assigned this Oct 6, 2022
@cizl cizl marked this pull request as ready for review October 6, 2022 10:01
@cizl cizl requested a review from tonilastre as a code owner October 6, 2022 10:01
Copy link
Contributor

@tonilastre tonilastre left a comment

Choose a reason for hiding this comment

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

Great refactoring! Now all events are structured as "event emitters" which is awesome!

Btw there is just one comment regarding name consistency.

Comment on lines 10 to 15
STABILIZATION_STARTED = 'stabilizationStarted',
STABILIZATION_PROGRESS = 'stabilizationProgress',
STABILIZATION_ENDED = 'stabilizationEnded',
NODE_DRAGGED = 'nodeDragged',
NODE_DRAG_ENDED = 'nodeDragEnded',
SETTINGS_UPDATED = 'settingsUpdated',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking, but now when you cleaned up Events in simulator engines and main classes, maybe it makes sense to make naming consistent with the rest of the library where:

  • Event values are present simple (e.g. render-start, node-hover, mouse-move)
  • Event values are not camel case (e.g. render-start, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! Makes sense. I'll also refactor these messages in the screenshot, as they are not written in MACRO_CASE.
Screenshot from 2022-10-07 12-59-49

or maybe the best example is this one where both are clearly visible:
Screenshot from 2022-10-07 13-03-15

Btw regarding naming, I have a few more questions:

  • We are using both stabilization and simulation. Should we merge those two?
  • In the picture above, should all of them be present simple?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say yes to both. Cleaner and consistent.

@cizl cizl requested a review from tonilastre October 7, 2022 13:40
Base automatically changed from fix-collapsing-canvas-size to release/0.1.2 October 7, 2022 15:51
@@ -27,6 +29,7 @@ export interface IDefaultViewSettings<N extends INodeBase, E extends IEdgeBase>
isOutOfBoundsDragEnabled: boolean;
areCoordinatesRounded: boolean;
isSimulationAnimated: boolean;
areCollapsedContainerDimensionsAllowed: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just don't forget to add this to the docs for both views.

@cizl
Copy link
Contributor Author

cizl commented Oct 10, 2022

@tonilastre Applied the standardized naming convention and updated docs. Feel free to take one final glance. 🙂

@cizl cizl changed the title Chore: Apply Emitter in simulator and fix cleanup when terminated Chore: Apply Emitter in simulator and fix cleanup when terminated, refactor naming Oct 10, 2022
@cizl cizl changed the title Chore: Apply Emitter in simulator and fix cleanup when terminated, refactor naming Chore: Apply Emitter in simulator and fix cleanup when terminated, Refactor naming Oct 10, 2022
Copy link
Contributor

@tonilastre tonilastre left a comment

Choose a reason for hiding this comment

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

Some minor fixes are needed.

}
tile: new L.TileLayer("https://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png"), // OpenStreetMaps
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Add areCollapsedContainerDimensionsAllowed here and to the interface above.

onStabilizationProgress: (data: ISimulatorEventGraph & ISimulatorEventProgress) => void;
onStabilizationEnd: (data: ISimulatorEventGraph) => void;
onSimulationStart: () => void;
onSIMULATION_PROGRESS: (data: ISimulatorEventGraph & ISimulatorEventProgress) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Khm :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was bound to happen 😅

@@ -49,7 +50,7 @@ export class DefaultView<N extends INodeBase, E extends IEdgeBase> implements IO

private _isSimulating = false;
private _onSimulationEnd: (() => void) | undefined;
private _simulationStartedAt = Date.now();
private _simulationStartedAtAt = Date.now();
Copy link
Contributor

Choose a reason for hiding this comment

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

Double At.

@cizl cizl requested a review from tonilastre October 10, 2022 10:32
@@ -204,7 +206,7 @@ that allows Orb to position the nodes.
![](./assets/view-default-fixed.png)

```typescript
import { DefaultView } from '@memgraph/orb';
import { DefaultView } from "@memgraph/orb";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to align our formatters at some point. Seems like every time we open a markdown file and save it we have different outputs.

@cizl cizl merged commit d078eb1 into release/0.1.2 Oct 10, 2022
@cizl cizl deleted the apply-emitter-in-simulator-and-fix-cleanup branch October 10, 2022 11:02
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