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

Integrate DetailviewExtension into multi-select views #3304

Merged
merged 2 commits into from May 2, 2019

Conversation

@lazyfrosch
Copy link
Member

commented Jan 23, 2018

Also make sure Javascript module handling will find the container for module auto-loading.

@lippserd Please review if this makes sense.

fixes #3072

@lazyfrosch lazyfrosch requested a review from lippserd Jan 23, 2018

@lazyfrosch

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2018

Here is an example that utilizes these updated hook:

bildschirmfoto von 2018-01-23 17-31-35
bildschirmfoto von 2018-01-23 17-31-26

@lazyfrosch lazyfrosch self-assigned this Jan 25, 2018

@lazyfrosch lazyfrosch assigned lippserd and unassigned lazyfrosch Feb 17, 2018

@lazyfrosch

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2018

@lippserd please review and merge

@lippserd
Copy link
Member

left a comment

Hi Markus,

Thanks for the PR. I requested some changes inline. Everything else looks good.

Cheers,
Eric

foreach (Hook::all('Monitoring\DetailviewExtension') as $hook) {
/** @var DetailviewExtensionHook $hook */
$module = $this->view->escape($hook->getModule()->getName());
$this->view->extensionsHtml[] =

This comment has been minimized.

Copy link
@lippserd

lippserd Jun 8, 2018

Member

The extension should only be added to the array if the hook does not return the empty string. Else we may render empty containers.

foreach (Hook::all('Monitoring\DetailviewExtension') as $hook) {
/** @var DetailviewExtensionHook $hook */
$module = $this->view->escape($hook->getModule()->getName());
$this->view->extensionsHtml[] =

This comment has been minimized.

Copy link
@lippserd

lippserd Jun 8, 2018

Member

The extension should only be added to the array if the hook does not return the empty string. Else we may render empty containers.

@lazyfrosch lazyfrosch self-assigned this Aug 22, 2018

lazyfrosch added some commits Jan 23, 2018

DetailviewExtension: Make sure data-icinga-module is set on container
To allow the eventhandler in JS to load and initialize the
corresponding module.

@lazyfrosch lazyfrosch force-pushed the feature/detailview-improvement branch from cc8d85e to 4753262 Aug 22, 2018

@lazyfrosch

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2018

Rebased and updated to your suggestions.

I also copied the "only output if content" check to the normal DetailviewExtension usage.

@lazyfrosch lazyfrosch added this to the 2.7.0 milestone Sep 13, 2018

@lazyfrosch lazyfrosch requested review from lippserd and nilmerg Apr 10, 2019

@lippserd
Copy link
Member

left a comment

Thanks for the changes. I though that it may be a good idea to not duplicate the "html generation" logic but have one function for that inside the hook, e.g. DetailviewExtensionHook::collectHtmlForObjects(). I decided against this because our new monitoring module may need different container HTML. So, this one is free to merge from my side.

@lippserd

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

@nilmerg Your feedback is still pending here.

@nilmerg

nilmerg approved these changes May 2, 2019

Copy link
Member

left a comment

LGTM

@nilmerg nilmerg merged commit 201cfa2 into master May 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nilmerg nilmerg deleted the feature/detailview-improvement branch May 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.