-
Notifications
You must be signed in to change notification settings - Fork 357
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
Remove ng-include in notifications #4813
Conversation
ng-include is problematic, started removing it in #4793, continuing
… of ng-include large template, so not inlining, but at least we can drop the extra async request for it Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1571223
Cc @skateman |
Checked commits https://github.com/himdel/manageiq-ui-classic/compare/94259d4dae5b047d5b55e57a865f9921e4a36a74~...97d83f2997d27951582d64c7da14e1bfc3ad9e6b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 **
|
'ng-repeat' => "notification in notificationGroup.notifications | limitTo:limit.notifications:(notificationGroup.notifications.length - limit.notifications)", | ||
'ng-include' => "notificationBodyInclude"} | ||
'ng-repeat' => "notification in notificationGroup.notifications | limitTo:limit.notifications:(notificationGroup.notifications.length - limit.notifications)"} | ||
= render :partial => 'static/notification_drawer/notification-body' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why this needs to be in a partial?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason at all, just didn't want to make the file unreadably large.
Should I just inline it too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, you don't have to!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove ng-include in notifications (cherry picked from commit ec520f4) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1571223
Hammer backport details:
|
In #4793 we started removing templates loaded asynchronously from other templates via ng-include.
Continuing with that - removing all remaining
ng-include
statements..titleInclude
was unused, removedheadingInclude
was used for a 2 line partial, inlinedsubheadingInclude
the samenotificationBodyInclude
replaced withrender :partial
so that we only have to do 1 request instead of 2(
notificationFooterInclude
inlined in #4793)The result of the 2 PRs is that we're now doing only 2 notifications-related requests on load (api, and template), instead of 6.
https://bugzilla.redhat.com/show_bug.cgi?id=1571223