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: progressive dashboard loading #244

Merged
merged 7 commits into from
Feb 19, 2019
Merged

Conversation

amcgee
Copy link
Member

@amcgee amcgee commented Feb 6, 2019

Fixes DHIS2-4867

This adds very basic support for progressive dashboard loading, so that only the dashboards visible in the current client window (scrolled into view) are loaded. Additional dashboards are loaded on scroll with a configurable debounce of 100ms.

In testing this reduces the number of initial HTTP requests spawned by a simple 8-item dashboard from 153 to 113 (with 4 dashboard items visible on startup) as seen in the screenshots below. (side note that's a whole lot of requests! We should reduce them. Also the dashboards javascript bundle is an astonishing 10 MB....)

screenshot 2019-02-06 16 04 14

screenshot 2019-02-06 16 05 11

Some cons of this approach and future enhancements:

  1. I'm just doing a dumb getBoundingClientRect check, so there might be cases where a dashboard is loaded even though only 1px of it is visible or the visible portion is covered by the header bar. I think it's OK to be a little over-zealous here for now.
  2. I'm also not currently checking left or right, but I don't think horizontally-scrolling dashboards are common (are they?)
  3. There is some render lag while scrolling. This was always the case if you scrolled while the items were loading, but it may happen more often now since there will always be items loading as you scroll (the first time). To fix this we would need to offload some processing to a service worker.
  4. Right now I've only done this with VisualizationItems, we might want to extend it also to AppItems?
  5. We might want to make this even more intelligent by queuing items and waiting for a certain number to load at a time before loading the rest. This could get annoying for fast-scrollers though.

@jenniferarnesen
Copy link
Collaborator

In response to your comments above:
1: I think its better to err on the side of loading too much
2. We don't need to worry about horizontal scrolling
3. Perceived performance on small to medium dashboards might be worse and I wonder if we could think up some solutions. On the flip side, it will actually be possible to load large dashboards, so maybe its worth releasing what we have now, and iteratively improving it.
4. I think just focusing on vis items for now is a great start and first iteration.

One thing - when user applies a filter, then all the vis items are loaded even if they are not in the viewport. So this doesn't fix those performance issues.

@amcgee
Copy link
Member Author

amcgee commented Feb 7, 2019

Thanks @jennifer, good catch of the filters issue. I will look at how to apply that fix, though it may be more involved

@amcgee
Copy link
Member Author

amcgee commented Feb 7, 2019

Updated to support an initialBufferFactor with a default of 0.5. This can be tuned to select how much of the second scroll viewport height is loaded on startup (so by default, anything visible in the first 150vh or 1.5 viewports is loaded).

For the Filter issue - progressive loading actually does work with filters, but each item is only loaded once. If the filter is updated, all items which have already loaded (so all of them if you've already scrolled through the whole dashboard, are updated at once. I will try to find a way to clear out the loaded state when updating the filter.

@cooper-joe
Copy link
Member

I think we should ensure that it's very obvious to the user why the items might not have loaded yet. As Jennifer mentioned, some users may perceive a slight decrease in performance. So let's make it as smooth as possible. As a minimum I think we should go for text inside all dashboard items saying "Loading item", that can be replaced when the item has fully loaded.

(I'm wary of spinners in this case because on older machines they can stall or just never load fully before the item loads anyway.)

@amcgee
Copy link
Member Author

amcgee commented Feb 7, 2019

I think we should ensure that it's very obvious to the user why the items might not have loaded yet. As Jennifer mentioned, some users may perceive a slight decrease in performance. So let's make it as smooth as possible. As a minimum I think we should go for text inside all dashboard items saying "Loading item", that can be replaced when the item has fully loaded.

(I'm wary of spinners in this case because on older machines they can stall or just never load fully before the item loads anyway.)

@joeDHIS there is an existing loading spinner which indicates a loading state, this change doesn't affect that. If we wanted to replace that spinner with something else I think that's outside the scope of this change. This only affects items which are not currently visible to the user, as soon as the item is scrolled into view the loading spinner appears - previously this happened while the item was off the screen.

There will be at most 100 milliseconds of delay before the loading indicator appears, but I think it would be more confusing to have text appear for such a brief period when there's a loading indicator about to appear.

If you were referencing the screenshot, I turned off the scroll event to take that screenshot to show the reduction in requests. That's why you see a blank white item.

@amcgee
Copy link
Member Author

amcgee commented Feb 7, 2019

For the record I think the spinners aren't the best choice here either, but they're what we currently have. As I mentioned to improve the performance of those spinners and prevent browsers from jerking when items are loading we'd need to significantly change the way we render individual items, again probably outside the scope of this change.

@amcgee
Copy link
Member Author

amcgee commented Feb 7, 2019

@jenniferarnesen I'm currently looking into the progressive re-loading after a filter change and also a potential bug with Chart items.

@cooper-joe
Copy link
Member

@amcgee Understood, thanks for the clear explanation 👍 I'll keep some ideas noted down for if/when we expand the scope of this implementation

@amcgee
Copy link
Member Author

amcgee commented Feb 7, 2019

@joeDHIS 👍

@jenniferarnesen I updated the PR to re-mount the progressiveLoading components whenever the itemFilter changes. To do this I generate a new key whenever the item changes, using a single-cache-depth memoized function (which I added to util.js and wrote tests for).

This makes for a nice, non-resource-intensive experience again when updating the filter after loading all items. It does reload items which might not respect the filter unnecessarily, but this was also the existing behavior.

The potential Chart bug was a misconfiguration of my environment (and a strange d2 behavior which is a crusade for another day)

@jenniferarnesen
Copy link
Collaborator

Looks good, tried it on Chrome, FF, and IE11. What do you think about moving the memoize fn into a separate file with a relevant name, rather than putting it in a nebulous file named util? Also, do you think the Progressive Container could use any unit tests?

@amcgee
Copy link
Member Author

amcgee commented Feb 18, 2019

@jenniferarnesen thanks for testing!

Sure, I like that idea. Will move the memoizeOne function to its own file (and move tests accordingly)

I thought about writing unit tests for the Progressive Container but I think they would be pretty fragile. The component is pretty tightly-coupled with window.scroll, so we would have to mock out a lot of the meat of the functionality to get a meaningful test, and then the test surface area is small. If we had Cypress set up in Dashboards I think it could be a good candidate for scroll testing, however. Let me know if you disagree and I can write the unit test, it shouldn't be terribly hard to do.

@jenniferarnesen
Copy link
Collaborator

Re tests for the Progressive Container - I agree, ran into the same issue when testing components that use refs to talk directly to the web api's. Let's prioritize getting Cypress into dashboards (not in this PR though :)

@amcgee
Copy link
Member Author

amcgee commented Feb 18, 2019

@jenniferarnesen @joeDHIS @geethaalwan This should be good to go. You can test it here, but be sure to log in to play/dev and clear your cache first.

Note that the first 1.5 viewport heights are loaded at first, so the easiest way to test it is to make your browser window pretty short. Then load a dashboard, wait for everything to complete, and note that you can interact with the visual dashboard items. Then scroll down and note that the previously-hidden items weren't loaded, so you'll see a loading spinner and then they will load when they come into view.

You can also validate this by looking at the "request count" in the Chrome console network tab, comparing it to the dashboards version on play (it should be less), and then seeing the number go up as you scroll down and more items load.

@jenniferarnesen
Copy link
Collaborator

@amcgee PR approved. As soon as Geetha has tested it, you can merge to master. Also, Scott had a question for you about backporting to 2.31 (see the Jira ticket).

When you merge, could you include in the commit description what was actually implemented (1.5 viewport heights, progressive loading also applies to when filters are added...)

@jenniferarnesen
Copy link
Collaborator

You can go ahead and merge. Netlify seems to be a little problematic with staying logged in and CORS issues.

@amcgee amcgee merged commit 19bd63b into master Feb 19, 2019
@amcgee amcgee deleted the feat/progressive-loading branch February 19, 2019 23:44
dhis2-bot added a commit that referenced this pull request Nov 26, 2020
# [31.1.0](v31.0.1...v31.1.0) (2020-11-26)

### Bug Fixes

* **translations:** sync translations from transifex (master) ([3e55743](3e55743))
* dashboard filter - filter dialog incorrectly shows filter as selected even though it was removed [DHIS2-9560] ([#1074](#1074)) ([54be7c3](54be7c3))
* flyoutMenu thinks every item is a flyout and stops propagation ([#1326](#1326)) ([326dd53](326dd53))
* **translations:** sync translations from transifex (master) ([08d2ca0](08d2ca0))
* **translations:** sync translations from transifex (master) ([32590e4](32590e4))
* **translations:** sync translations from transifex (master) ([f1b76c4](f1b76c4))
* **translations:** sync translations from transifex (master) ([2ef2caa](2ef2caa))
* **translations:** sync translations from transifex (master) ([9dc9a2a](9dc9a2a))
* **translations:** sync translations from transifex (master) ([a10cc54](a10cc54))
* **translations:** sync translations from transifex (master) ([fac8f80](fac8f80))
* **translations:** sync translations from transifex (master) ([e7982d6](e7982d6))
* **translations:** sync translations from transifex (master) ([838e1a1](838e1a1))
* **translations:** sync translations from transifex (master) ([d19c13f](d19c13f))
* **translations:** sync translations from transifex (master) ([3c4f359](3c4f359))
* **translations:** sync translations from transifex (master) ([b3b8a49](b3b8a49))
* **translations:** sync translations from transifex (master) ([bfb34df](bfb34df))
* **translations:** sync translations from transifex (master) ([1cbadba](1cbadba))
* **translations:** sync translations from transifex (master) ([#1295](#1295)) ([b8fdac9](b8fdac9))
* **translations:** sync translations from transifex (master) ([#1323](#1323)) ([26f5d8c](26f5d8c))
* force type COLUMN when viewing table as chart [DHIS2-9599] ([#1317](#1317)) ([fd2f1bb](fd2f1bb))
* ou item filter on App items was crashing Dashboards [DHIS2-9725] ([#1183](#1183)) ([614a42f](614a42f))
* restore schemas list needed for orgunitdlg and interpretations ([#1307](#1307)) ([4e3c83e](4e3c83e))
* switching from table to chart shows wrong data [DHIS2-9599] ([#1196](#1196)) ([ab60389](ab60389))
* try another format ([cc0df29](cc0df29))
* **translations:** sync translations from transifex (master) ([b4e24fb](b4e24fb))
* **translations:** sync translations from transifex (master) ([184a45e](184a45e))
* **translations:** sync translations from transifex (master) ([1cc0b5d](1cc0b5d))
* **translations:** sync translations from transifex (master) ([088e02a](088e02a))
* allow series in the request string ([#1148](#1148)) ([ef7d04e](ef7d04e))
* changing View As on one item was incorrectly causing other items to also change view [DHIS2-9590] ([#1111](#1111)) ([436b354](436b354))
* check whether the dashboard id has been set after the check for existing dashboards [DHIS2-9738] ([#1164](#1164)) ([05a8413](05a8413))
* use better spacing in item title when the title wraps to multiple lines ([#1099](#1099)) ([f76ed0e](f76ed0e))
* use correct prop name for D2Shim ([#1202](#1202)) ([5d521c6](5d521c6))
* **translations:** sync translations from transifex (master) ([b01a670](b01a670))
* add info in print preview about items that were shortened to fit on page [DHIS2-9423] ([#1045](#1045)) ([fde9ef1](fde9ef1))
* apply scrollbar to the dashboard area below the control bars and headerbar [DHIS2-9371] ([#1034](#1034)) ([ed049fe](ed049fe))
* bump Analytics to v11.0.5 and DV plugin to latest ([#1097](#1097)) ([b1d7fab](b1d7fab))
* console warning about invalid dom nesting ([#1049](#1049)) ([7039355](7039355))
* filter bar position (DHIS2-9453) ([#1073](#1073)) ([f573db0](f573db0))
* include title page in edit preview but scroll to below it [DHIS2-9417] ([#1038](#1038)) ([2c49410](2c49410))
* patch rgl with fix ([#1046](#1046)) ([33b046d](33b046d))
* print preview wasnt calculating page bottom correctly ([#1028](#1028)) ([505ad07](505ad07))
* remove unhelpful warning ([#1064](#1064)) ([d6e27e9](d6e27e9))
* save default dashboard title if user does not provide a title [DHIS2-9234] ([#1022](#1022)) ([32d390d](32d390d))
* scroll to top when switching dashboard ([#1040](#1040)) ([b7e6fab](b7e6fab))
* set overflow to hidden for vis and map items ([#1050](#1050)) ([d3b8d93](d3b8d93))
* set passive to true for scroll event listener in ProgressiveLoadingContainer [DHIS2-9508] ([#1084](#1084)) ([763881b](763881b))
* unmount map when switching dashboard mode or changing active type [DHIS2-9558] ([#1083](#1083)) ([147000e](147000e))
* upgrade plugin and analytics ([#1105](#1105)) ([db98d5c](db98d5c))
* **translations:** sync translations from transifex (master) ([0a41693](0a41693))
* **translations:** sync translations from transifex (master) ([491d592](491d592))
* setting selectedId to null caused endless spinner [DHIS2-9337] ([5e899a4](5e899a4))
* tweak info text and style ([#1048](#1048)) ([207530b](207530b))
* use hooks to exert finer control on setting dashboard [DHIS2-9508] ([#1067](#1067)) ([96f50f2](96f50f2))
* **translations:** sync translations from transifex (master) ([7badec8](7badec8))
* **translations:** sync translations from transifex (master) ([6a2bb23](6a2bb23))
* update analytics and dv-plugin ([#1039](#1039)) ([d726aae](d726aae))
* update dashboard scrollbar based on changes to window height [DHIS2-9427] ([#1047](#1047)) ([91999c7](91999c7))
* **translations:** sync translations from transifex (master) ([9dfdbfd](9dfdbfd))
* **translations:** sync translations from transifex (master) ([27a864d](27a864d))
* **translations:** sync translations from transifex (master) ([b1805ad](b1805ad))
* **translations:** sync translations from transifex (master) ([99a89e4](99a89e4))
* @dhis2/[email protected] ([#334](#334)) ([ffce4e5](ffce4e5))
* base url in production ([#1014](#1014)) ([4e63032](4e63032))
* **translations:** sync translations from transifex (master) ([7835db2](7835db2))
* add some more space above description ([#1004](#1004)) ([77dbf1f](77dbf1f))
* **translations:** sync translations from transifex (master) ([6097637](6097637))
* **translations:** sync translations from transifex (master) ([f0cf309](f0cf309))
* **translations:** sync translations from transifex (master) ([f849d52](f849d52))
* @dhis2/[email protected] ([#335](#335)) ([8e65eef](8e65eef))
* add missing reset css for lists ([#700](#700)) ([d73a618](d73a618))
* add title proptype for item header ([#525](#525)) ([1fef3e3](1fef3e3))
* [email protected]@34.3.34 ([#691](#691)) ([119e4f1](119e4f1))
* calculate item height accounting for long title [DHIS2-8492] ([#664](#664)) ([e1f73c9](e1f73c9))
* clear the item filters after the dashboard has been switched ([#612](#612)) ([8d8357f](8d8357f))
* cli-style commit hooks ([#520](#520)) ([f5bdc85](f5bdc85))
* conditionally show the Show More button in item selector ([#326](#326)) ([09d4c87](09d4c87))
* dashboard filter clear button ([#343](#343)) ([7e681d5](7e681d5))
* dashboards loading spinner (DHIS2-8384) + flash bug ([#613](#613)) ([ce2535b](ce2535b))
* [email protected] (DHIS2-8476) ([#662](#662)) ([b6b03c0](b6b03c0))
* [email protected] (DHIS2-8486) ([#663](#663)) ([4be5a31](4be5a31))
* dimensions as array (DHIS2-7787) ([#526](#526)) ([3647034](3647034))
* edit mode performance improvements ([#617](#617)) ([d8bcb58](d8bcb58))
* fall back to old types for visualizations ([#524](#524)) ([fbe93f4](fbe93f4))
* filter redesign ([#528](#528)) ([852459c](852459c))
* filter width + input bug ([#567](#567)) ([751cbc4](751cbc4))
* formatting ([#714](#714)) ([356a3a6](356a3a6))
* hide current vis type as option in 'view as' (DHIS2-8400) ([#616](#616)) ([38ae5fc](38ae5fc))
* hide scrollbar for chart items ([#337](#337)) ([a3b8848](a3b8848))
* hide titles for maps (DHIS2-8616) ([#718](#718)) ([2449234](2449234))
* hide vis action buttons for single value charts ([#336](#336)) ([b42812a](b42812a))
* interpretation comments delete DHIS2-8299 ([#541](#541)) ([6d2b568](6d2b568))
* List items not appearing to get added in edit mode (DHIS2-8567) ([#699](#699)) ([e06c4da](e06c4da))
* make dashboard items draggable again by passing down event props ([#523](#523)) ([86d7d3e](86d7d3e))
* Pass correct style prop to chart plugin on resize & update snapshots ([#299](#299)) ([07c6dec](07c6dec))
* pie tooltip DHIS2-7532 ([#349](#349)) ([fab7ab8](fab7ab8))
* [email protected] ([#593](#593)) ([6119bf2](6119bf2))
* prop types warnings DHIS2-8334 ([#569](#569)) ([381ce06](381ce06))
* remove unused dependency [TECH-316] ([#665](#665)) ([3a8e1a7](3a8e1a7))
* removes the 'view as' menu from gauge and pie ([#615](#615)) ([fd57417](fd57417))
* request program stage for dimensions ([#346](#346)) ([0d00250](0d00250))
* return the right url based on the report type ([#732](#732)) ([c707e93](c707e93))
* revert some css module changes and fix missing units ([#715](#715)) ([123c31d](123c31d))
* set filter badge top position based on offset height ([#301](#301)) ([70f248c](70f248c))
* set focus properties so textfield maintains focus when popover opens ([#297](#297)) ([771854e](771854e))
* set font-weight to 700 according to spec ([#618](#618)) ([e48e080](e48e080))
* update analytics/dv plugin dependencies ([#339](#339)) ([8796bb4](8796bb4))
* update dep for rich text parser fix ([#320](#320)) ([875e9ec](875e9ec))
* update plugin dep ([d837d8c](d837d8c))
* update plugin dep ([#661](#661)) ([febe988](febe988))
* updated dv-plugin for latest legend features ([#570](#570)) ([ae5c95f](ae5c95f))
* upgrade analytics so no ui-core mismatch ([#564](#564)) ([f154dbb](f154dbb))
* upgrade app-runtime-adapter-d2 to fix missing sharing translations ([#1016](#1016)) ([a0841ae](a0841ae))
* upgrade d2 to 31.8.1 ([#619](#619)) ([0671e1d](0671e1d))
* upgrade data-visualizer-plugin to 34.2.5 ([#566](#566)) ([1ccbd66](1ccbd66))
* upgrade DV plugin for single value support ([#332](#332)) ([0b643c7](0b643c7))
* upgrade dv plugin to v34.3.28 ([#623](#623)) ([6e9f30a](6e9f30a))
* upgrade [email protected] ([#620](#620)) ([6ac468b](6ac468b))
* upgrade ui-widgets to 2.1.1 for proper translations ([#851](#851)) ([47e098a](47e098a))
* use measured height of dashboard item in view mode [DHIS2-8492] ([#674](#674)) ([09658e3](09658e3))
* use the new visualizations api ([#591](#591)) ([a89c1cb](a89c1cb))
* use ui alertbar instead of material-ui ([#1149](#1149)) ([6fb0c99](6fb0c99))
* wrap VisPlugin with Fatal error boundary ([#621](#621)) ([1388a8d](1388a8d))
* **filters:** dimension items selection ([#278](#278)) ([8344c4a](8344c4a))
* **filters:** filter the list of dimensions usable as filters ([#287](#287)) ([6569200](6569200))
* **filters:** wrap badges + clear/disable filters on dashboard switch and edit mode ([#280](#280)) ([ad3e3c3](ad3e3c3))
* **styles:** fix styles for filter selector ([#308](#308)) ([3da2b30](3da2b30))
* **TextItem:** use all the vertical space for the input ([#283](#283)) ([9419d28](9419d28))

### Features

* add support for multi root ou in filters ([#345](#345)) ([31dd611](31dd611))
* buffer while scrolling with progressive loading. ([#273](#273)) ([7bea49b](7bea49b))
* condense item header options into a single menu ([#568](#568)) ([993981b](993981b))
* dashboards filters ([#275](#275)) ([bc22817](bc22817))
* Enable epi-weekly periods in filter ([#331](#331)) ([c3d6afc](c3d6afc))
* fetch items chart data on app level ([#271](#271)) ([a2e3e94](a2e3e94))
* implements latest Dynamic Dimension selector from Analytics (DHIS2-8831) ([#885](#885)) ([f878995](f878995)), closes [dhis2/data-visualizer-app#1046](dhis2/data-visualizer-app#1046)
* implements latest Period Selector from Analytics (DHIS2-8807) ([#872](#872)) ([886ccce](886ccce))
* print dashboard [DHIS2-7045] ([#1015](#1015)) ([20cb64f](20cb64f))
* progressive dashboard loading ([#244](#244)) ([19bd63b](19bd63b))
* Show "more button" disabled instead of hiding it ([#323](#323)) ([28396c3](28396c3))
* upgrade to ui@5 (TECH-385) ([#891](#891)) ([d551b81](d551b81)), closes [dhis2/analytics#476](dhis2/analytics#476)
* use the new visualizations type instead of report tables and charts ([#540](#540)) ([8742a6a](8742a6a))

### Reverts

* remove file inadvertently added to master before its time ([#1264](#1264)) ([b6d80f3](b6d80f3))
* Revert "fix: use better spacing in item title when the title wraps to multiple lines (#1099)" (#1112) ([5a0b3b2](5a0b3b2)), closes [#1099](#1099) [#1112](#1112)
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 31.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants