Icons on app inspector#140
Conversation
(there is one non-commented one left, where we want to WARN but don't have $log available, which i've left)
and support non-autogeneration in icon pipe
| } | ||
| } | ||
| .node-icon { | ||
| .pull-right(); |
There was a problem hiding this comment.
Why using .pull-right() mixin where the whole block is using flexbox? This is not right, should be one or the other. Based on the current setup, I think this should use flexbox only)
There was a problem hiding this comment.
as it's non-growable and the other field is growable, we don't need it at all, quite right
| <span class="node-icon"><img ng-src="{{ iconUrl }}"/></span> | ||
| </a> | ||
| <div class="entity-node-toggle" ng-if="entity.children.length > 0 || entity.members.length > 0" ng-click="onToggle($event)" > | ||
| <div class="entity-node-toggle-wrapper"> |
There was a problem hiding this comment.
Why wrapping this into another div? This entire "row" (i.e. entity status, entity name, icon and expand toggle) can be handled with flexbox which would be cleaner and easier to implement, rather than doing it with unnecessary HTML layout.
There was a problem hiding this comment.
the ng-if hides the toggle on leaf nodes. but we want the icons to align so we need a placeholder/wrapper.
| } | ||
|
|
||
| export function entityNodeDirective() { | ||
| export function entityNodeDirective($log, iconService) { |
There was a problem hiding this comment.
Seems to not be necessary. The $log service is not used and the iconService is injected on the controller level
There was a problem hiding this comment.
correct. also removed the inject line
|
|
||
| } | ||
| } | ||
| entityNodeDirective.$inject = ['$log', 'iconService']; |
| <brooklyn-status-icon value="{{entity.serviceState}}" ng-if="entity.serviceState || entity.applicationId"></brooklyn-status-icon> | ||
| <i class="fa fa-2x fa-external-link" ng-if="!entity.serviceState && !entity.applicationId"></i> | ||
| <span>{{entity.name}}</span> | ||
| <span class="node-icon"><img ng-src="{{ iconUrl }}"/></span> |
There was a problem hiding this comment.
Should use the entity.iconUrl instead of adding this into the scope, so the icon will be saved on the model which is propagated everywhere.
There was a problem hiding this comment.
nice idea but i think $scope.entity can be updated by the caller, so we'd need to watch for it to prevent it being cleared. it isn't usually the icon which updates, it's things like status, so watching for that and reloading icon seems overkill.
| function controller ($scope, $state, $stateParams, iconService) { | ||
| $scope.isOpen = true; | ||
|
|
||
| iconService.get($scope.entity, true).then(value => $scope.iconUrl = value); |
There was a problem hiding this comment.
Should set the entity.iconUrl instead of adding this into the scope, so the icon will be saved on the model which is propagated everywhere.
| vm.name = response.data.name; | ||
| vm.error.entity = undefined; | ||
| })); | ||
| iconService.get(response.data, true).then(value => vm.iconUrl = value); |
There was a problem hiding this comment.
Should set the entity.iconUrl instead of adding this into the scope, so the icon will be saved on the model which is propagated everywhere.
| function IconService($q, $http, iconGenerator, $log, cache) { | ||
| this.get = getIcon; | ||
| function getIcon(id) { | ||
| function getIcon(entityOrTypeId, doNotAutogenerate) { |
There was a problem hiding this comment.
As far as I can see, the there is no called using the doNotAutogenerate. It seems to be used only on the pipe.
There was a problem hiding this comment.
it's used in entity-tree.directive.js:
iconService.get($scope.entity, true).then(value => $scope.iconUrl = value);
|
all addressed screen-shot:
|
|
merging now, as discussed with 1:1 review @tbouron |
3 participants
show icons on app-inspector tree view and summary, to help navigate/understand
also optimize/tidy icon generation code