Feature: ComputedProperty Dependent keys in debugger & services highlighting #781

lifeart · 2018-04-09T20:05:08Z

blue - computed property deps
bold green - services

it will be greate to get some view-component like
http://basisjs.com/basisjs/demo/data/data-flow.html

nummi · 2018-04-09T23:56:04Z

Just playing with some ideas. In this mockup I went the Apple route (Safari and Xcode) with icons that just have a letter. Not sure if this is a good idea for people where English isn't their first language. But overall I think it's easily scannable by the eyes.

I do want to be careful not to make the Object Inspector too busy.

A variation without lines:

Another:

lolmaus · 2018-04-10T06:34:39Z

This is a fantastic, very useful feature!

However, I believe displaying dependency trees inline is inefficient, since they can be arbitrarily deep and branchy.

I'd rather have a button next to each CP that would pop up a modal with a full-screen interactive dependency tree, similar to how @lahmatiy's Basis.JS does it:

However, I don't remember seeing any modals in Ember Inspector, and I'm not sure if it's even possible.

lifeart · 2018-04-10T10:54:16Z

@nummi codepen for icons

https://codepen.io/lifeart/pen/jzdzZm

path? M 150 50 Q 150 100 200 100 L 300 100 Q 350 100 350 150

summerisgone · 2018-04-10T13:18:25Z

@lifeart https://codepen.io/summerisgone/pen/bvzvKB

nummi · 2018-04-10T16:25:53Z

However, I believe displaying dependency trees inline is inefficient, since they can be arbitrarily deep and branchy.

Makes sense. Maybe the immediate dependency is okay for a first pass?

rwjblue · 2018-04-10T17:05:51Z

ember_debug/object-inspector.js

+        options.dependentKeys = hash[prop]._dependentKeys;
+        if (!options.isService) {
+          try {
+            options.code = hash[prop]._getter.toString();


Since you always expect this to be a function, could you essentially do Function.prototype.toString.call(hash[prop]._getter)?

@rwjblue no luck( Tests not passed

lifeart · 2018-04-10T18:43:01Z

Features:

toggling / [c] button click, by default: toggled
cp code / [c] button hover

icon sizing update

summerisgone · 2018-04-11T06:15:57Z

app/styles/object_inspector.scss

@@ -10,3 +10,107 @@
  margin-left: 39px;
  word-break: break-all;
 }
+
+:root {


Probably not here, but in the top of the file?

summerisgone · 2018-04-11T06:40:32Z

app/styles/object_inspector.scss

+}
+
+.mixin__property-dependency-item:first-child::before {
+  background: url('data:image/svg+xml;utf8,<svg width="17" height="15" version="1" xmlns="http://www.w3.org/2000/svg"><g fill="none" fill-rule="evenodd" stroke="#979797"><circle cx="13" cy="11" r="3"/><path d="M13 8c0-3-1-4-4-4H5C2 4 1 3 1 0"/></g></svg>') no-repeat;


Let's try
background: url('data:image/svg+xml;utf8,<svg width="17" height="15" xmlns="http://www.w3.org/2000/svg"><g transform="translate(1)" stroke="#979797" fill="none" fill-rule="evenodd"><circle fill="#FFF" cx="12" cy="11" r="3"/><path d="M12 8c0-3-1-4-4-4H4C1 4 0 3 0 0"/></g></svg>') no-repeat;

data:image/svg+xml;utf8,<svg doesn't seem to working for me in Firefox

summerisgone · 2018-04-11T06:41:11Z

app/styles/object_inspector.scss

+.mixin__property-dependency-item::before {
+  content: "";
+  display: block;
+  background: url("data:image/svg+xml;utf8,<svg width='8' height='16' xmlns='http://www.w3.org/2000/svg'><g transform='translate(-14 -16)' stroke='#979797' fill='none' fill-rule='evenodd'><circle cx='18' cy='28' r='3'/><path d='M18 24.571V16.5'/></g></svg>") no-repeat;


Longer arrow and white circle
<svg width="8" height="20" xmlns="http://www.w3.org/2000/svg"><g transform="translate(1 -1)" stroke="#979797" fill="none" fill-rule="evenodd"><circle fill="#FFF" cx="3" cy="17" r="3"/><path d="M3 13.571V1.489649"/></g></svg>

lifeart · 2018-04-17T17:25:52Z

It’s ready for review

lifeart · 2018-04-17T21:09:04Z

@rwwagner90 new tests added

nummi · 2018-04-17T21:28:42Z

@lifeart, is toggling the display of the dependencies broken? Clicking the "C" isn't working for me.

lifeart · 2018-04-17T21:43:08Z

@nummi, (it's fixed in my last commit with tests), Openable "C" property must have bold name. If name not "bold" - no dependent keys

nummi · 2018-04-17T22:00:42Z

I wonder if we made the list a little more difficult for the eyes to scan. Right now the UI needs some spacing and alignment adjustments — which will help — but I want to tinker in Photoshop a little more. Thoughts?

Screenshot 1 is everything expanded; 2 is every collapsed; 3 is current.

lifeart · 2018-04-17T22:14:19Z

After 5 mins of comparing I can't say that my eyes tired about implemented part.
I think we need more space alignment between last expanded dependency and property under it.
As variant - we can remove "bold" from services, and replace icons to symbols with redued saturation.

lolmaus · 2018-04-18T06:12:47Z

@nummi:
I wonder if we made the list a little more difficult for the eyes to scan. Right now the UI needs some spacing and alignment adjustments — which will help — but I want to tinker in Photoshop a little more. Thoughts?

I agree with @nummi here:

The icons are too heavy and obtrusive, compared to the original calculator icon, which, I assume, represented computed ("calculated") properties. What's the reason for abandoning the existing icon? To my taste, the result is amateurish and gaudy.
Everything is bold now. Again, quite obtrusive. The existence of CP dependencies can be indicated by, for example, a triangle icon which is a universally accepted UI pattern for an expandable element, widely used in Ember Inspector already. Using bold to indicate expandability is wrong, UX-wise.
This may already be the case, but I believe all CPs should be collapsed by default. Otherwise, this listing can get unnecessarily long and hard to grasp.

PS Dependency listings are awesome! Are they clickable?

lifeart · 2018-04-18T06:23:21Z

PS Dependency listings are awesome! Are they clickable?

Nope, right now they not clicable, there is some reasons for it:

CP dependency can be located in another mixin, we need to expand it and scroll user into it - this is not good UX for me
CP deps may look like foo.@each{a,b,c}, should we open foo and use default serialization, or we need to apply specific serialization approach for foo, to get only a, b, c keys for it's children?

teddyzeenny · 2018-04-18T09:09:34Z

app/styles/object_inspector.scss

+  padding: 0;
+  list-style: none;
+  display: block;
+  // margin-bottom: 0.2em;


Forgotten comment.

teddyzeenny · 2018-04-18T10:02:40Z

ember_debug/object-inspector.js

+  prop.dependentKeys = options.dependentKeys || [];
+  let hasServiceFootprint = prop.value && typeof prop.value.inspect === 'string' ? prop.value.inspect.includes('@service:') : false;
+  prop.isService = options.isService || hasServiceFootprint;
+  prop.code = options.code;


Can you add tests for services and DKs here?

@teddyzeenny done

tests for cp-expand & service highlight valueType alias css comment removal cp deps and service serialization keys test is service detection update

nummi · 2018-04-26T19:20:07Z

@lifeart, I have my own branch for playing with the UI. I have everything lined up and the SVG rendering pixel perfect. Let me know if you want to integrate this code.

(working with SVG and this small of a scale is a nightmare)

nummi · 2018-04-26T19:24:40Z

Also, this is the current state of my mockups:

summerisgone · 2018-04-27T05:34:16Z

I agree with @nummi that new UI looks more heavy, but let's compare new proposal on the same screen, with more elements

lifeart · 2018-04-27T08:09:36Z

@nummi, looks awesome! Sure, I want to integrate it! Could you open pr against https://github.com/lifeart/ember-inspector/tree/computed-dependent-keys and I will mege it, to add your stuff to this PR

teddyzeenny · 2018-04-28T15:32:53Z

Hey! What's the status of this? I'm planning to release the inspector this weekend, do you think it can make it?

lifeart · 2018-04-28T15:41:51Z

@teddyzeenny I'm waiting for @nummi PR, for me all looks good

nummi · 2018-04-28T15:57:17Z

Hey! What's the status of this? I'm planning to release the inspector this weekend, do you think it can make it?

@teddyzeenny @lifeart, getting my commit ready now.

nummi · 2018-04-28T16:24:33Z

@teddyzeenny @lifeart, here is the PR:
lifeart#3

Tested in Firefox and Chrome on macOS.

lifeart · 2018-04-28T16:42:37Z

@nummi @teddyzeenny, merged, waiting for green CI flag

lifeart · 2018-04-28T16:56:52Z

@nummi @teddyzeenny all green, I think thi PR ready for acceptance

Dismissing old review, since changes were made

RobbieTheWagner

LGTM! Thanks for all the hard work! 🎉

lolmaus · 2018-04-29T08:00:06Z

Congrats!

Though this question was left unanswered:

The icons are too heavy and obtrusive, compared to the original calculator icon, which, I assume, represented computed ("calculated") properties. What's the reason for abandoning the existing icon?

nummi · 2018-04-29T19:42:56Z

The letter icons are more about consistency. We are highlighting services in addition to computed properties; we struggled with an icon for services and two styles of icons looked even more busy. I guess we'll see how the community Reacts. Maybe I've been staring at the new design too long but I think it works well when looking at the inspector as a whole.

…ighting (emberjs#781)

draft

16ed712

lifeart changed the title ~~Feature: ComputedProperty Dependent keys in debugger~~ WIP / Feature: ComputedProperty Dependent keys in debugger Apr 9, 2018

service detection and cp code catching

0201761

lifeart changed the title ~~WIP / Feature: ComputedProperty Dependent keys in debugger~~ WIP / Feature: ComputedProperty Dependent keys in debugger & services highlighting Apr 9, 2018

better service detection

b9f42b0

lifeart added 2 commits April 10, 2018 08:11

safer service detection

aa8e891

cleanup template

70df144

try catch for code getter to handle serialization issue

a35b25f

lifeart mentioned this pull request Apr 10, 2018

Null check for calculateToString method emberjs/ember.js#16478

Closed

rwjblue reviewed Apr 10, 2018

View reviewed changes

lifeart added 3 commits April 10, 2018 20:13

Merge branch 'master' into computed-dependent-keys

96e8f1c

code to string update from comments

9d13376

code cleanup

1930c40

lifeart changed the title ~~WIP / Feature: ComputedProperty Dependent keys in debugger & services highlighting~~ Feature: ComputedProperty Dependent keys in debugger & services highlighting Apr 10, 2018

lifeart added 6 commits April 10, 2018 21:54

style cleanup

fd6cb32

code cleanup

3ff991f

hasDependent keys computed

9d97c7e

template code cleanup

effe144

icon sizing update

5379176

Merge pull request #2 from lifeart/pr/781

e93aa93

icon sizing update

summerisgone reviewed Apr 11, 2018

View reviewed changes

lifeart force-pushed the computed-dependent-keys branch from 6e3c35a to c04384d Compare April 15, 2018 10:14

teddyzeenny reviewed Apr 18, 2018

View reviewed changes

lifeart force-pushed the computed-dependent-keys branch from e38f033 to cf31208 Compare April 18, 2018 20:22

updates from comments and eslint fix

8a496c5

tests for cp-expand & service highlight valueType alias css comment removal cp deps and service serialization keys test is service detection update

lifeart force-pushed the computed-dependent-keys branch from cf31208 to 8a496c5 Compare April 18, 2018 20:24

Merge branch 'master' into computed-dependent-keys

86a438c

Style adjustment to object properties (#3)

81cdb55

RobbieTheWagner approved these changes Apr 29, 2018

View reviewed changes

RobbieTheWagner merged commit e6015b1 into emberjs:master Apr 29, 2018

cyril-sf pushed a commit to cyril-sf/ember-inspector that referenced this pull request Mar 30, 2022

Feature: ComputedProperty Dependent keys in debugger & services highl…

be27cec

…ighting (emberjs#781)

Feature: ComputedProperty Dependent keys in debugger & services highlighting #781

Feature: ComputedProperty Dependent keys in debugger & services highlighting #781

Conversation

lifeart commented Apr 9, 2018 • edited Loading

nummi commented Apr 9, 2018 • edited Loading

lolmaus commented Apr 10, 2018 • edited Loading

lifeart commented Apr 10, 2018 • edited Loading

summerisgone commented Apr 10, 2018

nummi commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifeart commented Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

summerisgone Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

summerisgone Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifeart commented Apr 17, 2018

lifeart commented Apr 17, 2018

nummi commented Apr 17, 2018 • edited Loading

lifeart commented Apr 17, 2018

nummi commented Apr 17, 2018

lifeart commented Apr 17, 2018

lolmaus commented Apr 18, 2018

lifeart commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nummi commented Apr 26, 2018 • edited Loading

nummi commented Apr 26, 2018

summerisgone commented Apr 27, 2018

lifeart commented Apr 27, 2018

teddyzeenny commented Apr 28, 2018

lifeart commented Apr 28, 2018

nummi commented Apr 28, 2018

nummi commented Apr 28, 2018 • edited Loading

lifeart commented Apr 28, 2018

lifeart commented Apr 28, 2018

RobbieTheWagner left a comment

Choose a reason for hiding this comment

lolmaus commented Apr 29, 2018

nummi commented Apr 29, 2018

lifeart commented Apr 9, 2018 •

edited

Loading

nummi commented Apr 9, 2018 •

edited

Loading

lolmaus commented Apr 10, 2018 •

edited

Loading

lifeart commented Apr 10, 2018 •

edited

Loading

lifeart commented Apr 10, 2018 •

edited

Loading

summerisgone Apr 11, 2018 •

edited

Loading

summerisgone Apr 11, 2018 •

edited

Loading

nummi commented Apr 17, 2018 •

edited

Loading

nummi commented Apr 26, 2018 •

edited

Loading

nummi commented Apr 28, 2018 •

edited

Loading