95 #104

Merged
merged 7 commits into from Jan 10, 2017

Projects

None yet

2 participants

@micmro
Owner
micmro commented Jan 8, 2017

First part of #95 to make the overlays work with multiple instances of PerfCascade.

There is some more work required to get the paging working as well (waterfall-docs-service.ts and the modules that rely on it), but I did not want to make it too large.

This implantation still looks a bit odd, I was wondering if it makes sense to create a context or so object that we pass around in function calls since pretty much all modules are stateless and have no notion of instances. @tobli do you have any ideas?

added some commits Jan 8, 2017
@micmro fix function typing and formating 80cbf62
@micmro #95 make overlay pub-sub work with multiple instances
13d113a
@micmro micmro requested a review from tobli Jan 8, 2017
src/ts/typing/open-overlay.d.ts
+ * @param {SVGSVGElement} chartBaseEl - base chart element of the chart instance that has triggered the update
+ * @returns void
+ */
+export type OverlayChangeSubscriber = (change: OverlayChangeEvent, chartBaseEl: SVGSVGElement) => void;
@tobli
tobli Jan 8, 2017 Collaborator

chartBaseEl is not used in any OverlayChangeSubscriber, as far as I can tell.

@micmro
micmro Jan 9, 2017 Owner

I left it there for potential future use, but I think of refactoring this pr and see what the context approach would look like.

src/ts/typing/open-overlay.d.ts
+ */
+export type OverlayChangeSubscriber = (change: OverlayChangeEvent, chartBaseEl: SVGSVGElement) => void;
+
+export type EventTypes = "closed" | "open";
@tobli
tobli Jan 8, 2017 Collaborator

For a type I think a singular form is better (EventType). This also clashes with eventTypes in overlay-changes-pub-sub.ts (I think that one should be removed).

@micmro
micmro Jan 9, 2017 Owner

good point.

src/ts/helpers/svg.ts
+ if (el.classList.contains(baseCssClassName)) {
+ return el as SVGSVGElement;
+ }
+ return getBaseEl(el.parentElement) as SVGSVGElement;
@tobli
tobli Jan 8, 2017 Collaborator

This casting is fishy. Parent elements aren't typically elements. The element with a class that matches baseCssClassName will be (by convention). Maybe a separate (non-exported) function that recurses based on Elements, then cast the result of that to SVGSVGElement.

@micmro
micmro Jan 9, 2017 Owner

But it's just the result of getBaseEl(el.parentElement) that gets casted as SVGSVGElement - and the result is always a SVGSVGElement - else it throws an error. Though in fairness we could omit it, since the function signature takes care of this.

@tobli
tobli Jan 9, 2017 Collaborator

Right you are!
Tobias vs. Recursion: 0 - 1

- subscribers.forEach((fn) => fn(change));
-}
+export function publishToOverlayChanges(change: OverlayChangeEvent, overlayHolder: SVGGElement) {
+ let eventChartBaseEl = getBaseEl(overlayHolder);
@tobli
tobli Jan 8, 2017 Collaborator

Is it possible to skip passing the overlayHolder here, and pass it at subscription time? You'd make subscribers a map from chart id to subscriber array, and pass in the id you'd like to subscribe to events for there. That makes the potentially heavy operation (as the code might not have that information readily available) occur once, rather that doing a tree traversal every time an event is published.

@micmro
micmro Jan 9, 2017 Owner

The problem is that at the component are not attached to any parent at subscription time, this is why I have solved it this way. However eventChartBaseEl is not used by any subscriber - and can be removed.

I've timed the traversal for 58 subscribers and it was less than a millisecond.

- "openOverlays": openOverlays,
+ "combinedOverlayHeight": getCombinedOverlayHeight(overlayHolderId),
+ "openOverlays": openOverlays[overlayHolderId],
+ "overlayHolderId": overlayHolderId,
"type" : overlayChangesPubSub.eventTypes.CLOSE,
@tobli
tobli Jan 8, 2017 Collaborator

This is already type checked to be one of the strings "open" or "closed" (in https://github.com/micmro/PerfCascade/pull/104/files#diff-6d03add939cb5983af028f4f400c1a57R16). Just put "closed" here, and remove overlayChangesPubSub.eventTypes.

@@ -46,11 +48,14 @@ export function closeOverlay(index: number, overlayHolder: SVGGElement,
export function openOverlay(index: number, y: number, accordionHeight: number, entry: WaterfallEntry,
overlayHolder: SVGGElement, barEls: SVGGElement[]) {
- if (openOverlays.filter((o) => o.index === index).length > 0) {
+ const overlayHolderId = overlayHolder.id;
+ if (openOverlays[overlayHolderId] && openOverlays[overlayHolderId].filter((o) => o.index === index).length > 0) {
@tobli
tobli Jan 8, 2017 Collaborator

Maybe this is more readable as two if statements, and with some(). Something like:

if (!openOverlays[overlayHolderId]) {
      openOverlays[overlayHolderId] = [];
}
if (openOverlays[overlayHolderId].some((o) => o.index === index)) {
      return;
}
- "openOverlays": openOverlays,
+ "combinedOverlayHeight": getCombinedOverlayHeight(overlayHolderId),
+ "openOverlays": openOverlays[overlayHolderId],
+ "overlayHolderId": overlayHolderId,
"type" : overlayChangesPubSub.eventTypes.OPEN,
@tobli
tobli Jan 8, 2017 Collaborator

Just use "open" here (see above).

@tobli
Collaborator
tobli commented Jan 8, 2017

Good progress! But I think it's going to remain a bit tricky with the current setup, requiring knowledge about the parent hierarchy to setup publishers/subscribers. How would it be if the modules that have state (directly or via a global object) were converted into proper classes? (e.g. overlay-changes-pub-sub and svg-details-overlay-manager). It seems they are configured from svg-chart anyway, where the information about this particular chart is available.

@micmro
Owner
micmro commented Jan 8, 2017
added some commits Jan 9, 2017
@micmro refactor eventTypes
9929a45
@micmro improve readability
b72ddd2
@micmro normalize file extension f091211
@micmro refactor use context object
5e062f8
@micmro remove unneded files
419c4ed
+ options: ChartOptions;
+ /** Is the main document TLS/SSL */
+ docIsSsl: boolean;
+}
@micmro
micmro Jan 10, 2017 Owner

new object that can be passed around to inject state.

+ public closeOverlay: (index: number, accordionHeight: number, barEls: SVGGElement[]) => void;
+
+ constructor(context: Context, overlayHolder: SVGGElement);
+}
@micmro
micmro Jan 10, 2017 Owner

Might move theses somewhere else later

@@ -1,20 +1,15 @@
// simple pub/sub for change to the overlay
@micmro
micmro Jan 10, 2017 edited Owner

Changed to class. In the future we could make this generic, e.g. once we have the need for another pub/sub adding a channel name parameter.

@@ -1,115 +1,122 @@
+import {Context, OverlayManagerClass} from "../../typing/context";
@micmro
micmro Jan 10, 2017 Owner

Changed to class.

@tobli
tobli approved these changes Jan 10, 2017 View changes

Nice! Looking good, I've tested and it seems to work fine. I updated the index.html to include two waterfalls by default, and had to make a few minor tweaks to the demo html/css (switching from ids to classes). But I can push those after this is merged.

@micmro micmro merged commit faffa83 into master Jan 10, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@micmro micmro deleted the 95 branch Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment