22 #30

SuperConfuserUser · 2019-09-24T20:34:27Z

Added static designs for the vApp visual component.

SuperConfuserUser · 2019-09-25T05:48:34Z

src/components/entity-label.ts

+    // handled by the parent
+    if (this._fontWeight) {
+      super.setFontWeight('bold');
+    }


feels like there may be a better way of changing label's font style and having the label background update.

most of the questions for any label type components are essentially based on how to inherit or derive from the base LabelComponent.

we could probably add an on('frame') event handler for the label component that checks if clipping/resizing is needed on every frame.. then text properties could be updated naturally without having to worry about ensuring that the clipping and sizing are updated explicitly. (Or every N frames if the logic is too expensive, but it looks cheap to me at the moment).

setting all text properties in a LabelTextComponent first. the label background draws afterwards without having to update

SuperConfuserUser · 2019-09-25T05:50:30Z

src/components/isolated-network-label.ts

+  }
+
+  // TODO: maxWidth and clip is reused from the generic label component. is there a better way to share these?
+  /**


would it be better to extend this component from the LabelComponent? the label style is fairly different and it doesn't need the label background at all

it would be nice to share this logic in one way or another. if this component doesn't need most of what the label component offers, it might be good to break out an even more abstract component for just the text clipping piece, that can be extended by boht.

new LabelTextComponent handles text styling and clipping

SuperConfuserUser · 2019-09-25T05:52:55Z

src/components/label.ts

+    this._label.fontWeight = weight;
+    this.clip();
+    this._background.bounds.width = this._label.bounds.width + (LABEL_HORIZONTAL_PADDING * 2);
+  }


would it be better to make this more generic and pass in a configuration object? ex. setFontStyle({ weight: 'bold' })

if we keep it like this, then yeah probably so. I'm thinking the frame event handler is probably the way to go here, though

SuperConfuserUser · 2019-09-25T06:03:06Z

src/components/vapp.ts

+    // vms and vnics list
+    this.vms = new VmAndVnicListComponent(
+      this._vapp.vms,
+      this.vappNetworks && this.vappNetworks.networkPositionsByName,


passing 'networkPositionsByName' derived from vappNetworks component here. is there a better way or naming convention to use? more documentation? // networkPositionsByName have x position data for matching vnics

it was clear to me but might be helpful to be more explicit about the fact that it's the X position. maybe networkXPositionsByName

since it's clear, i'll leave it as is. there's a possibility that positions might need to be converted (local, global, parent, child) later on, so i'll keep full position data

SuperConfuserUser · 2019-09-25T06:04:52Z

src/components/vapp.ts

+
+    // finish VappNetworks - draw the top external segment and internal segment based on matching vnic positions
+    if (vappNetworkCount) {
+      this.vappNetworks.setTopAndBottomSegments(this.vms.lowestVnicPointByNetworkName);


passing 'lowestVnicPointByNetworkName' derived from vmsAndVnicList component here. is there a better way or naming convention to use? more documentation? // lowestVnicPointByNetworkName contains the lowest point data for matching vapp networks

made sense to me, so I think it's OK

SuperConfuserUser · 2019-09-25T06:07:57Z

src/components/vapp.ts

+ * Virtual Application Visual Component.
+ */
+export class VappComponent extends paper.Group {
+


this is the major parent component that encapsulates everything else and handles most of the interdependent and shared data

SuperConfuserUser · 2019-09-25T06:10:36Z

src/components/vm-and-vnic-list.ts

+    // reusable calculations
+    const vappNetworkCount = Object.keys(this.vappNetworkPositionsByName).length;
+    this.lastNetworkPositionX = vappNetworkCount && (vappNetworkCount - 1) * VAPP_NETWORK_RIGHT_MARGIN
+      + CONNECTOR_RADIUS - DEFAULT_STROKE_WIDTH;


could also loop through vappNetworkPositionsByName from line 36 to find max position instead of calculating it like this. does it matter?

actually, there's an even simpler way of getting it from the bounds of the VappNetworkListComponent from the main VappComponent.

yeah, probably better to get it that way vs. having this calculation that is dependent on other, previous calulations

SuperConfuserUser · 2019-09-30T14:35:19Z

src/components/vapp.ts

+        this._vapp.vapp_networks,
+        new paper.Point(VAPP_PADDING + CONNECTOR_RADIUS, 0));
+      this.addChild(this.vappNetworks);
+    }


max (furthest right) vapp network position can be derived from this component's bounds to pass to the vmAndVnicListComponent

nevermind, edge labels can throw the width off

simplifying even more by using the value straight from the vappNetworkList.lastNetworkPosition

cfsnyder

I finished an initial review but I need to come back and look at a few things more closely when I have time. Nice job, looking good so far!

cfsnyder · 2019-09-30T15:59:49Z

...src/app/components/misc-scrollbar-demo-component/misc-scrollbar-horizontal-demo.component.ts

    );
+    if (scrollbar.isEnabled) {
+      canvas.onmouseenter = scrollbar.containerMouseEnter;


I don't like this pattern where you're requiring the parent component to bind the event handlers. That should be internal to the scrollbar at the time it is created. I suggest creating a wrapper for the canvas that you're rendering to which exposes Observables that are bound to those event handlers so that any component can easily subscribe to them.

event handlers are internal to the scrollbar now

cfsnyder · 2019-09-30T16:03:18Z

demo/src/app/components/vapp-static-demo-component/vapp-static-demo.component.ts

+
+    const content = new paper.Group({ applyMatrix: false });
+    // create origin paper Item for vapps to base position from
+    const origin = new paper.Path.Circle({


what is this about?

VApps base their x position on the last child of the content group (line 44 - content.lastChild.bounds.right). Most vApps will have the previous vApp to refer to, but the very first one won't. The origin was created to be the first vApp's reference point. One way it could also be avoided is by doing a check in the loop at line 43:

vapps.forEach((vappData, index) => { const position = new paper.Point(index === 0 ? 'explicit number' : content.lastChild.bounds.right), VERTICAL_POSITION); ... }

cfsnyder · 2019-09-30T16:07:52Z

src/components/scrollbar.ts

+  /**
+   * Gets isEnabled state. The scrollbar is enabled when content does not fit the container.
+   */
+  get isEnabled(): boolean {


typically a boolean field on a class (or a property accessor, like you have here) would just be named enabled. Names like isEnabled are usually used for getter methods

cfsnyder · 2019-09-30T16:13:40Z

demo/src/styles.less

@@ -1,4 +1,7 @@
 /* You can add global styles to this file, and also import other style files */
+
+@import (css) url('https://fonts.googleapis.com/css?family=Roboto:400,500,700&display=swap');


where are we at with this? did you end up handling the issues that we talked about, where the font loads too slow and calculation of component positioning is based upon the wrong font sizes? Perfectly fine if that's still a TODO, I'm just curious

yes. still a TODO right now. The delayed font loading seems to affect the label background sizes for the most part and could be solved with the onFrame solution suggested earlier. Another option I was thinking of looking into was using the Angular lifecycle methods to wait for the font to load completely before rendering the canvas. Or have an observable/promise that waits for the font to load, and then redraw. There are a few other font loading techniques too that I haven't dug into yet.

cfsnyder · 2019-09-30T16:14:09Z

src/components/connector.test.ts

+import { VAPP_BACKGROUND_COLOR, CANVAS_BACKGROUND_COLOR } from '../constants/colors';
+import * as paper from 'paper';
+
+describe('connector component', () => {


cfsnyder · 2019-09-30T17:41:11Z

src/components/vapp-network-list.ts

+   * Clones the bottom segment of vapp networks to separate for scrolling and removes the original segment.
+   * @param splitPositionY vertical point where the network path should be split for cloning and separation
+   */
+  cloneVmListSegments(splitPositionY: number) {


missing return type

cfsnyder · 2019-09-30T17:50:40Z

src/components/vapp.ts

+    // vms and vnics list
+    this.vms = new VmAndVnicListComponent(
+      this._vapp.vms,
+      this.vappNetworks && this.vappNetworks.networkPositionsByName,


it was clear to me but might be helpful to be more explicit about the fact that it's the X position. maybe networkXPositionsByName

cfsnyder · 2019-09-30T17:52:48Z

src/components/vapp.ts

+
+    // finish VappNetworks - draw the top external segment and internal segment based on matching vnic positions
+    if (vappNetworkCount) {
+      this.vappNetworks.setTopAndBottomSegments(this.vms.lowestVnicPointByNetworkName);


made sense to me, so I think it's OK

cfsnyder · 2019-09-30T17:53:56Z

src/components/vapp.ts

+   */
+  // TODO: add scroll listener with a better method. it's in parent demo component for now. could create a native canvas
+  //  event observable service similar to the paper.event service?
+  setScrollListening(event: WheelEvent) {


yeah we definitely need to do something different for these event bindings, this is going to get out of hand like this

cfsnyder · 2019-09-30T18:03:15Z

src/components/vm-and-vnic-list.ts

+    // reusable calculations
+    const vappNetworkCount = Object.keys(this.vappNetworkPositionsByName).length;
+    this.lastNetworkPositionX = vappNetworkCount && (vappNetworkCount - 1) * VAPP_NETWORK_RIGHT_MARGIN
+      + CONNECTOR_RADIUS - DEFAULT_STROKE_WIDTH;


yeah, probably better to get it that way vs. having this calculation that is dependent on other, previous calulations

SuperConfuserUser · 2019-09-30T19:57:23Z

src/components/vapp.ts

+   * Clips and adds scrolling to the VmAndVnicList component when it's too large for the view.
+   */
+  private clipAndScrollVmList() {
+    // create drop shadow at the top of the vm list that fades in or out onScroll


add return type

SuperConfuserUser · 2019-09-30T19:58:18Z

src/components/vm-and-vnic-list.test.ts

+    return expectedVnicPoints;
+  }
+
+  test.only('basic properties and vnics on different networks', () => {


remove only

from all the tests

SuperConfuserUser · 2019-10-02T15:22:02Z

@mathieupkf - Could you also review the visuals? Netlify Demo

The request to display scrollbars only on scroll area hover has also been implemented. There's a lot going on for visibility and opacity with the scrollbar system. Trying to add the 1s delay/fade in with all of that made the visuals very buggy unfortunately.

The extra edge cases that aren't explicitly in the InVision designs are for a visual check on some of the layout logic. Let me know if the different vApp cases could be arranged better too. Thanks!

mathieupkf · 2019-10-03T12:57:14Z

@SuperConfuserUser - thanks Chely.
The scrollbar apparition is really nice. Can we add an opacity delay ? 0 to 100%. Duration 0.3s.
I have another suggestion for the scroll area (I'm not 100% sure so keep what we have right now). The shadow element looks a little bit too strong, I don't really like it. Can we try without it? Can we try by starting the scroll area just below the vApp Name / the Edge Name?

SuperConfuserUser · 2019-10-04T14:38:03Z

@mathieupkf - a few updated scrollbar demos ready. 2 or 3 would be my personal pick. The more subtle drop shadow would be a nice touch to show the fold. Can do more variations as well if you have more ideas.

mathieupkf · 2019-10-09T07:59:26Z

Thanks @SuperConfuserUser !
I'm not 100% sure yet... This scroll area looks a little bit odd to me, don't you think? I have a last suggestion and I'll make my final choice after that ;)
Can you create another variation where the whole height of the vApp will be scrollable ? vApp titles / Edge titles will scroll like the content.

SuperConfuserUser · 2019-10-24T21:39:47Z

@mathieupkf - Thanks for the review! The scrollbar dragging bug is fixed in this demo. We can definitely explore more scrollbar improvements when the project picks back up.

SuperConfuserUser · 2019-10-28T22:51:42Z

demo/src/app/constants/vapp-static-placeholder-data.ts

-            network_name: '',
-            is_connected: false
+            network_name: 'A',
+            is_connected: true
          },


testing a case for when connected and disconnected vnics are out of order

SuperConfuserUser · 2019-10-28T23:02:19Z

src/components/vm-and-vnic-list.ts


    // draw vms and their vnics
    this._vms.forEach((vmData, index) => {
-      const pointY = (LABEL_HEIGHT + VM_MARGIN_VERTICAL) * index;
+      const sharedPointY = (LABEL_HEIGHT + VM_MARGIN_VERTICAL) * index;
+      let xPositionMultiplier = hasVappNetworks ? 1 : 0;


this would also work as let xPositionMultiplier = hasVappNetworks since it's a boolean, but i wanted to be super explicit about where values come from

SuperConfuserUser force-pushed the 22 branch from 9408176 to 4034463 Compare September 25, 2019 05:04

Closes ilanddev#22: add vapp static designs

e54cc54

SuperConfuserUser force-pushed the 22 branch from 4034463 to e54cc54 Compare September 25, 2019 05:39

SuperConfuserUser commented Sep 25, 2019

View reviewed changes

SuperConfuserUser commented Sep 30, 2019

View reviewed changes

cfsnyder requested changes Sep 30, 2019

View reviewed changes

SuperConfuserUser commented Sep 30, 2019

View reviewed changes

SuperConfuserUser force-pushed the 22 branch from 98b64a8 to 4c42376 Compare October 9, 2019 15:23

updates

d8924f5

SuperConfuserUser force-pushed the 22 branch from 4c42376 to d8924f5 Compare October 23, 2019 23:24

SuperConfuserUser commented Oct 28, 2019

View reviewed changes

SuperConfuserUser requested a review from cfsnyder October 28, 2019 23:07

		@@ -1,4 +1,7 @@
		/* You can add global styles to this file, and also import other style files */

		@import (css) url('https://fonts.googleapis.com/css?family=Roboto:400,500,700&display=swap');

22 #30

Are you sure you want to change the base?

22 #30

Conversation

SuperConfuserUser commented Sep 24, 2019

SuperConfuserUser Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfsnyder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperConfuserUser commented Oct 2, 2019

mathieupkf commented Oct 3, 2019

SuperConfuserUser commented Oct 4, 2019

mathieupkf commented Oct 9, 2019 • edited Loading

SuperConfuserUser commented Oct 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperConfuserUser Sep 25, 2019 •

edited

Loading

mathieupkf commented Oct 9, 2019 •

edited

Loading