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.

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

feat(monitoring): SD-3123 Added fetch as panel on monitoring view #1135

Merged
merged 5 commits into from
Sep 22, 2015

Conversation

pavlovicnemanja
Copy link
Contributor

SD-3123

@akintolga
Copy link
Collaborator

👍

@@ -1431,6 +1431,8 @@
},
templateUrl: 'scripts/superdesk-authoring/views/send-item.html',
link: function sendItemLink(scope, elem, attrs) {
var parentScope = scope.$parent.$parent;
Copy link

Choose a reason for hiding this comment

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

If I understand correctly, this grandparent scope is only referenced to obtain a reference to the monitoring controller used by the MonitoringViewDirective? If so, this pattern should be avoided, because it is fragile and sensitive to changes in the scope hierarchy.

The right way to do it is to declare that this directive depends on the parent MonitoringViewDirective (require: '^sdMonitoringView'), and then the monitoring controller will be passed to the link() function as its 4th argument:

function sendItemLink(scope, elem, attrs, ctrl) {
    // ctrl === MonitoringController

Copy link
Contributor

Choose a reason for hiding this comment

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

agree with @plamut .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I know, and in first way I do it like this. But (always some but), this directive is also used for sending item on another desk in authoring view. As there is no sdMonitoringView directive found by require in authoring screen, it throws error.

This way was my alternative.

Copy link

Choose a reason for hiding this comment

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

Hm... so how does the monitoring controller end up in the parent scope then, what places it there? Would it be possible to somehow reference that thing and obtain the controller reference from it?

@petrjasek @amagdas Are there any other good alternatives to obtaining the controller from the grandparent scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Monitoring Controller is accessible with require on monitoring page. But it is not accessible on authoring page. So, as this two pages use same directive for sending item, on monitoring view works fine, but on authoring it throws an error. Because of that I added
if (parentScope.monitoring) { .

Copy link

Choose a reason for hiding this comment

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

Yeah, that's clear and perfectly fine... I'm just wondering if there exists a better way, that's all :)

Copy link
Member

Choose a reason for hiding this comment

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

seems like it's only setting some css classes - you can use superdeskFlags service for that:

superdeskFlags.flags.fetching = !!item;

it will add fetching class to #main-container

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, great :) ... I didn't saw this service :)

@@ -8,7 +8,7 @@ <h3 translate ng-show="mode === 'archive'">Duplicate</h3>
<div class="content">
<div class="publish-pane-box">
<div sd-toggle-box data-title="{{ 'Destination' | translate }}" data-open="true"
ng-if="mode === 'ingest' || (mode === 'authoring' && $root.isActionAllowed(item, 'move') && itemActions.send)">
ng-if="mode === 'ingest' || mode === 'monitoring' || (mode === 'authoring' && $root.isActionAllowed(item, 'move') && itemActions.send)">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please extract long conditionals in the controller with a proper function name.

@ioanpocol
Copy link
Contributor

👍
One e2e test is needed

@ioanpocol
Copy link
Contributor

👍

@amagdas
Copy link
Contributor

amagdas commented Sep 22, 2015

👍

amagdas added a commit that referenced this pull request Sep 22, 2015
feat(monitoring): SD-3123 Added fetch as panel on monitoring view
@amagdas amagdas merged commit 53fabbc into superdesk:master Sep 22, 2015
@pavlovicnemanja pavlovicnemanja deleted the fetchAs branch October 2, 2015 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants