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.

Sign up for GitHub

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

Support tracked and other improvements #1035

Merged
merged 12 commits into from
Oct 23, 2019
Merged

Support tracked and other improvements #1035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
feaab82
Support tracked and other improvements
patricklx Sep 23, 2019
fb8f7a3
Merge branch 'master' into support-refresh.same
RobbieTheWagner Oct 14, 2019
178a2b6
add workaround for [object AsyncFunction]
patricklx Oct 14, 2019
e242dc7
Merge branch 'support-refresh.same' of github.com:patricklx/ember-ins…
patricklx Oct 14, 2019
ae32f42
improve production build detection of tracked and class names
patricklx Oct 18, 2019
0dbedc7
fix
patricklx Oct 22, 2019
69b8107
extract into helper
patricklx Oct 22, 2019
08c31ec
revert to previous inheritance/merging semantics
Oct 23, 2019
5e1ce0c
improve class name and inspector view
patricklx Oct 23, 2019
57ff01f
use classname only with ember toString
Oct 23, 2019
3f5300a
add more tests for new behavior
Oct 23, 2019
b533e96
getter compat
Oct 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 8 additions & 25 deletions app/components/object-inspector/property.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,29 +19,11 @@
{{/if}}

{{!-- Property Icon --}}
{{#if this.isService}}
<span
class="mixin__property-icon-container"
title="Service"
>
<span class="mixin__property-icon mixin__property-icon--service"></span>
</span>
{{else if this.isComputedProperty}}
<span
class="mixin__property-icon-container"
title={{if @model.code @model.code "code not avaliable"}}
{{on "click" this.toggleDeps}}
>
<span class="mixin__property-icon mixin__property-icon--computed"></span>
</span>
{{else}}
<span
class="mixin__property-icon-container"
title="Property"
>
<span class="mixin__property-icon mixin__property-icon--property"></span>
</span>
{{/if}}
<span class="mixin__property-icon-container" title={{if @model.code @model.code this.iconInfo.title}}
{{on "click" this.toggleDeps}}
>
<span class="mixin__property-icon mixin__property-icon--{{this.iconInfo.type}}"></span>
</span>

{{!-- Property Name --}}
<span class="mixin__property-name js-object-property-name">
Expand Down Expand Up @@ -88,15 +70,16 @@
/>
{{/if}}
{{else}}
{{#if (and this.isComputedProperty (not this.isCalculated))}}
{{#if this.canCalculate}}
<button
class="mixin__calc-btn js-calculate"
title="compute property"
{{on "click" @calculate bubbles=false}}
>
{{svg-jar "calculate" width="16px" height="16px"}}
</button>
{{else if this.isDate}}
{{/if}}
{{#if this.isDate}}
<span
class="{{this.valueType}} mixin__property-value js-object-property-value"
{{on "click" this.dateClick}}
Expand Down
47 changes: 42 additions & 5 deletions app/components/object-inspector/property.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ export default Component.extend({
txtValue: null,
dateValue: null,

isCalculated: computed('valueType', function () {
return this.valueType !== 'type-descriptor';
}),
isCalculated: alias('model.value.isCalculated'),

valueType: alias('model.value.type'),

Expand All @@ -30,7 +28,7 @@ export default Component.extend({

isInstance: equal('valueType', 'type-instance'),

isComputedProperty: alias('model.value.computed'),
isComputedProperty: alias('model.isComputed'),
Copy link
Member

Choose a reason for hiding this comment

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

Does this not need to be model.value.isComputed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

model.isComputed now indicates this is is a Ember.computed property. And instead it will use model.value.isCalculated to check if it was calculated.
Somehow I feel the value.computed was used for both is Ember.computed and isCalculated
https://github.com/emberjs/ember-inspector/pull/1035/files/96efd0d7e7e98103fcd44e6a696b238d57d6e3df#diff-7c2c7ea1dfc0ef384d1ec6e685711d15L455


isFunction: equal('valueType', 'type-function'),

Expand All @@ -46,6 +44,42 @@ export default Component.extend({

showDependentKeys: and('isDepsExpanded', 'hasDependentKeys'),

canCalculate: computed('model', 'isCalculated', 'isComputedProperty', function () {
if (this.get('isOverridden')) return false;
if (!this.get('isComputedProperty') && this.get('model.isGetter') && this.get('model.isExpensive')) {
return true;
}
return this.get('isComputedProperty') && !this.get('isCalculated');
}),

iconInfo: computed('isService', 'model.inspect.value', 'model.isTracked', 'model.isProperty', 'model.isGetter', 'isFunction', function () {
if (this.get('isService')) {
return { type: 'service', title: 'Service' };
}

if (this.get('isFunction')) {
return { type: 'function', title: 'Function' };
}

if (this.get('model.isTracked')) {
return { type: 'tracked', title: 'Tracked' };
}

if (this.get('model.isProperty')) {
return { type: 'property', title: 'Property' };
}

if (this.get('model.isComputed')) {
return { type: 'computed', title: 'Computed' };
}

if (this.get('model.isGetter')) {
return { type: 'getter', title: 'Getter' };
}

return { type: 'n/a', title: 'N/A' };
}),

canDig() {
return this.isInstance
|| this.isObject
Expand All @@ -54,11 +88,14 @@ export default Component.extend({
},

cannotEdit() {
if (!this.isCalculated) return true;
return this.isFunction || this.isOverridden || this.readOnly;
},

toggleDeps: action(function () {
this.toggleProperty('isDepsExpanded');
if (this.hasDependentKeys) {
this.toggleProperty('isDepsExpanded');
}
}),

valueClick: action(function () {
Expand Down
54 changes: 41 additions & 13 deletions app/components/object-inspector/sort-properties.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,55 @@
import Component from '@ember/component';
import { sort } from '@ember/object/computed';
import { sort, map } from '@ember/object/computed';
import { set, computed } from '@ember/object';
import { A } from '@ember/array';

export default Component.extend({
tagName: '',

isArray: computed('properties', function () {
const props = A(this.get('properties') || []);
return props.findBy('name', 'length') && props.findBy('name', 0);
}),

/**
* Sort the properties by name to make them easier to find in the object inspector.
* Sort the properties by name and group them by property type to make them easier to find in the object inspector.
*
* @property sortedProperties
* @type {Array<Object>}
*/
sortedProperties: sort('properties', 'sortProperties'),
sortedProperties: sort('props', 'sortProperties'),

init() {
this._super(...arguments);
props: map('properties', function (p) {
set(p, 'isFunction', p.value.type === 'type-function');
if (p.name == parseInt(p.name)) {
set(p, 'name', parseInt(p.name));
}
return p;
}),

/**
* Used by the `sort` computed macro.
*
* @property sortProperties
* @type {Array<String>}
*/
this.sortProperties = ['name'];
},
/**
* Used by the `sort` computed macro.
*
* @property sortProperties
* @type {Array<String>}
*/
sortProperties: computed('isArray', function () {
const order = [
'isFunction',
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious, how did you decide the ordering for sorting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mmm, a mix between how it looks like (most times) in the code and whats useful:

  1. services are always at the top
  2. then comes properties, tracked
  3. later computed and getters, -> but I think computed/getters and tracked are more interesting than normal properties, so there are more at the top
  4. finally functions at the bottom (sometimes useful if one wants to debug the function, then send to console and click on the function and chrome will send you directly to the source code) (i would even prefer if there was a goto for functions instead of send to console :) )

So, final order is:
Service -> Tracked -> Computed -> Getter -> Property -> Function

the sortProperties is more a short trial & error :)
had to put isFunction at the top, since its also a isProperty

Copy link
Member

Choose a reason for hiding this comment

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

I think the order should be:

Service -> Property -> Tracked -> Computed -> Getter  -> Function

That mimics the recommended ordering from eslint-plugin-ember of the order we should define things in our component JS files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mmm, i think this is what I had initially too.
But for arrays i wanted to see the computed properties like lastObject & firstObject at the beginning and not scroll down 100 items...
But this is only in case of arrays and 2 CPs :) .
So, if you prefer the order similar to eslint-plugin-ember, i go ahead and change it

Copy link
Member

Choose a reason for hiding this comment

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

I think maybe we should follow eslint-plugin-emebr, but you make a good point, so perhaps for arrays we could have a special case that changes the order?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think special casing arrays would be fine, this could also be done in a followup though. I agree that properties and tracked fields should likely be grouped together, the eslint rule seems to make the most sense. I'd say let's do that to get this merged, then followup to improve the story for arrays specifically.

'isService:desc',
'isProperty:desc',
'isTracked:desc',
'isComputed:desc',
'isGetter:desc',
'name'
];
// change order for arrays, if the array doesnt have items, then the order does not need to be changed
if (this.get('isArray')) {
const i = order.indexOf('isProperty:desc');
order.splice(i, 1);
order.splice(order.length - 1, 0, 'isProperty:desc');
}
return order;
})
});

12 changes: 9 additions & 3 deletions app/routes/application.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,15 @@ export default Route.extend({
},

updateProperty(options) {
const detail = this.get('controller.mixinDetails.mixins').objectAt(options.mixinIndex);
const property = get(detail, 'properties').findBy('name', options.property);
set(property, 'value', options.value);
if (this.get('controller.mixinDetails.mixins')) {
const detail = this.get('controller.mixinDetails.mixins').objectAt(options.mixinIndex);
let property = get(detail, 'properties').findBy('name', options.property);
if (!property) return;
set(property, 'value', options.value);
if (options.dependentKeys) {
set(property, 'dependentKeys', options.dependentKeys);
}
}
},

updateErrors(options) {
Expand Down
2 changes: 2 additions & 0 deletions app/styles/colors.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
--pill-bg: rgba(0, 0, 0, 0.1);
--pill-bg-active: rgba(255, 255, 255, 0.3);
--warning-text: #735c0f;
--tracked-icon-bg: var(--spec03);
}

.theme--dark {
Expand Down Expand Up @@ -78,4 +79,5 @@
--pill-bg: rgba(255, 255, 255, 0.2);
--pill-bg-active: rgba(255, 255, 255, 0.3);
--warning-text: #8ca3f0;
--tracked-icon-bg: #5e5eb5;
}
24 changes: 24 additions & 0 deletions app/styles/object_inspector.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,3 +126,27 @@
content: "P";
}
}

.mixin__property-icon--getter {
background-color: var(--spec07);

&:before {
content: "G";
}
}

.mixin__property-icon--function {
background-color: var(--base08);

&:before {
content: "ƒ";
}
}

.mixin__property-icon--tracked {
background-color: var(--tracked-icon-bg);

&:before {
content: "T";
}
}
2 changes: 1 addition & 1 deletion app/templates/components/object-inspector.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
<button
class="send-to-console js-send-object-to-console-btn"
title="Send to Console"
{{on "click" (fn this.sendObjectToConsole @model.firstObject)}}
{{on "click" (fn this.sendObjectToConsole @model.lastObject)}}
RobbieTheWagner marked this conversation as resolved.
Show resolved Hide resolved
>
{{svg-jar "send-with-text" width="20px" height="10px"}}
</button>
Expand Down
Loading