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

Notifications - fix UI performance with thousands of notifications #611

Merged
merged 17 commits into from
Apr 21, 2017
Merged

Notifications - fix UI performance with thousands of notifications #611

merged 17 commits into from
Apr 21, 2017

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Mar 7, 2017

This (hopefully temporarily) moves notification area from patternfly to manageiq, and fixes it to have better performance characteristics with thousands of notifications...

WIP, now it actually uses our copy of the code, no other changes yet

  • not render the notifications until the user actually expands the list (use ng-if instead of ng-show)
  • limit the number of notifications we're retrieving from the server on load to as sane number (add ?limit=100 to the API request)
  • limit the number of notifications we show in the list (for situations when we downloaded a 100, but then got another 3000 via websocket) - or use a infiniscroll component to do that

https://bugzilla.redhat.com/show_bug.cgi?id=1412172
(FINE: https://bugzilla.redhat.com/show_bug.cgi?id=1448045)
(EUWE: https://bugzilla.redhat.com/show_bug.cgi?id=1448046)

@himdel
Copy link
Contributor Author

himdel commented Mar 8, 2017

Adding a bunch of notifications on js side..

s = angular.element($0).scope();
for (var i = 0; i < 1024; i++) {
  s.notificationGroup.notifications.push({
    timeStamp: new Date(),
    data: {
      timeStamp: new Date(),
      endTime: new Date(),
      startTime: new Date(),
      message: "Foo!" + i,
    }
  });
}
s.$apply();

when seeding from the API, ask for only the last 100 notifications, instead of all of them
…hoses otherwise

when notification count > 100 (in a group), the user sees only the first 100, and there's an extra button to show all of them - and to go back to showing only a 100
@himdel
Copy link
Contributor Author

himdel commented Apr 19, 2017

Less than a hundred items (no change):

not-few

More than a hundred - default:

not-100

More than a hundred - showing all

not-all

@himdel himdel removed the wip label Apr 19, 2017
@himdel himdel changed the title [WIP] Notifications - fix UI performance with thousands of notifications Notifications - fix UI performance with thousands of notifications Apr 19, 2017
@miq-bot
Copy link
Member

miq-bot commented Apr 19, 2017

Checked commits https://github.com/himdel/manageiq-ui-classic/compare/e333d14893e924138a214dcca8adbe707d44ff54~...2b58e8f025a0b24ba3364e9fec24d2e97d94598f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 2 offenses detected

app/views/static/notification_drawer/notification-drawer.html.haml

  • ⚠️ - Line 26 - Line is too long. [195/160]
  • ⚠️ - Line 38 - Do not use semicolons to terminate expressions.

@himdel
Copy link
Contributor Author

himdel commented Apr 19, 2017

Only the last 4 commits actually change stuff, the rest just brings in stuff from patternfly and cleans it up :).

@h-kataria h-kataria added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 21, 2017
@h-kataria h-kataria merged commit 40eeb17 into ManageIQ:master Apr 21, 2017
@himdel himdel deleted the spam-notify branch April 24, 2017 09:11
simaishi pushed a commit that referenced this pull request Apr 24, 2017
Notifications - fix UI performance with thousands of notifications
(cherry picked from commit 40eeb17)
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 542673998dfe010bc32371805ed6df6233edffee
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Fri Apr 21 17:22:54 2017 -0400

    Merge pull request #611 from himdel/spam-notify
    
    Notifications - fix UI performance with thousands of notifications
    (cherry picked from commit 40eeb17970f77b54679c5937b93f5527a53530c7)

@simaishi
Copy link
Contributor

Euwe backport (to manageiq repo) details:

$ git log -1
commit a3c50981c059d8128c0696e3cb389e67d4f0c225
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Fri Apr 21 17:22:54 2017 -0400

    Merge pull request #611 from himdel/spam-notify
    
    Notifications - fix UI performance with thousands of notifications
    (cherry picked from commit 40eeb17970f77b54679c5937b93f5527a53530c7)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1448046

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants