fix: attempt at fixing re-render when interpretations panel toggles #2199
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Not successful so far.
|
🚀 Deployed on https://pr-2199--dhis2-dashboard.netlify.app |
jenniferarnesen
approved these changes
Jan 5, 2023
This causes unnecessary re-renders when the interpretations panel is toggled.
jenniferarnesen
approved these changes
Jan 5, 2023
edoardo
added a commit
that referenced
this pull request
Jan 24, 2023
edoardo
added a commit
that referenced
this pull request
Mar 2, 2023
janhenrikoverland
pushed a commit
that referenced
this pull request
Mar 3, 2023
* chore: convert to functional component * feat: enable EVENT_VISUALIZATION dashboard item types * feat: allow iframe plugin overrides for dev/test * feat: render LL plugin in iframe * chore: add dependency for post-robot * chore: update pot file * fix: lookup in installed apps for the plugin URL to use in the iframe * chore: remove TODO comment * chore: update pot file * chore: add handy deduplicate yarn command * fix: fix ui dependency * chore: run prettier * refactor: rewrite condition for better readability * fix: remove Suspense which might interfere with the iframe * fix: ensure the selector content fits in the container This is a ui issue: dhis2/ui#1152. * refactor: use Layer + Popper to avoid bottom double border * fix: remove redundant overflow rule, fixed in ui This reverts commit a36ab37. * chore: deduplicate yarn.lock * refactor: preserve height/width if provided, update props passed * refactor: use iframe for DV plugin * chore: remove unused import * feat: add recording props to plugin (#2125) * fix(pwa): make cache filters more specifc * feat(plugins): add `record` prop to iframe plugins when caching dashboard * feat: remove cached plugin data with dashboard * chore: run Prettier * test: fix failing tests * chore: remove unused code * chore: run Prettier * fix: pass correct prop for display property Also do not pass the whole userSettings object, not used in the plugins where the only thing really needed is the displayProperty for the analytics request. * chore: deduplicate yarn.lock * fix: add more paths to omit from cache Replaced deprecated config key with new one. * fix: show error when iframe src is not available Also avoid adding postRobot handlers when the iframeRef is not available, this removes some extra errors. * fix: do not pass filtered AO to LL plugin Filters don't work in LL without modifications. For now just disable filtering for LL items. * fix: set error when pluginLaunchUrl is not available The app could be installed and the entry in apps api returned, but the plugin launch URL can be missing, in that case the plugin cannot be loaded in the iframe. * refactor: use specific reducer selector * fix: remove View As in context menu for LL items * fix: clear selected from Redux when editing a new dashboard * feat: add tags on item header, used for filters in LL For LL items we don't apply filters. Show a tag in the item header to indicate this to the user. * feat: add overlay on LL items to inform about not applied filters * refactor: move the overlay handling to the parent component In this way both the tag in the item header and the overlay in the item content can be controlled at the same time allowing for toggling the tag depending on whether the overlay is shown or not. * test: update snapshots * fix: remove tag icon and fix styles Approved by Joe. * chore: update pot file * fix: remove context menu when plugin is not installed * fetch schema for eventVisualizations, required by the interpretations component * fix: rename Event visualizations to Line lists This appears as section title in the dashboard item search. * feat: add custom message for when the plugin is not installed More friendly than the generic error message. In this case there isn't any problem with the dashboard item, but the plugin for rendering the item is not installed. We inform the user about it and suggest to install the missing app from the app hub. * refactor: propagate d2 needed to check installed apps * feat: use iframe for Map items * chore: update d2-ui-interpretations component * fix: update iframe src and props when using View As * chore: fix linting issue * fix: fix re-render of visualizations when interpretations panel toggles (#2199) * fix: add app id for DV when fetched from installed apps * fix: detect plugin availability via plugin launch URL * fix: add fallback to bundled plugin path for core apps This addresses the case of core apps plugins which are always served from the bundled app's path, instead of from api/apps. For core (bundled) apps: * first we look at the api/apps, if there is an entry there for the app it means it has been manually installed from the app hub overriding the bundled version. The plugin URL in this case is not in the format api/apps but dhis-web-*. * if no entry is returned from api/apps we use the "hardcoded" path (as before) thus using the bundled version of the plugin. This should work for situations where a new version of a core app is installed or uninstalled from the app hub. * chore: remove accidentally committed temp files * fix: use base url without api version This should solve the issue of bundled apps plugin not working in iframes. * fix: deduplicate yarn.lock * fix: attempt at fixing the sizing issue in DV iframe content * feat: map plugin (#2229) --------- Co-authored-by: Kai Vandivier <[email protected]> Co-authored-by: Jen Jones Arnesen <[email protected]>
janhenrikoverland
pushed a commit
that referenced
this pull request
May 24, 2023
* chore: convert to functional component (cherry picked from commit 4f0abd8) * feat: enable EVENT_VISUALIZATION dashboard item types (cherry picked from commit 1dd409c) * feat: allow iframe plugin overrides for dev/test (cherry picked from commit f79f622) * feat: render LL plugin in iframe (cherry picked from commit 29c5397) * chore: add dependency for post-robot (cherry picked from commit 9300b81) * chore: update pot file (cherry picked from commit cd4ef81) * fix: lookup in installed apps for the plugin URL to use in the iframe (cherry picked from commit e733903) * chore: remove TODO comment (cherry picked from commit 1e0f641) * chore: update pot file (cherry picked from commit 7144805) * chore: add handy deduplicate yarn command (cherry picked from commit f38c3b1) * fix: fix ui dependency (cherry picked from commit b887981) * chore: run prettier (cherry picked from commit b857e9c) * refactor: rewrite condition for better readability (cherry picked from commit 010be92) * fix: remove Suspense which might interfere with the iframe (cherry picked from commit d33336b) * fix: ensure the selector content fits in the container This is a ui issue: dhis2/ui#1152. (cherry picked from commit 322dde1) * refactor: use Layer + Popper to avoid bottom double border (cherry picked from commit cd0751d) * fix: remove redundant overflow rule, fixed in ui This reverts commit a36ab37. (cherry picked from commit f4cad7b) * chore: deduplicate yarn.lock (cherry picked from commit 2b4ad23) * refactor: preserve height/width if provided, update props passed (cherry picked from commit e2fca17) * chore: remove unused import (cherry picked from commit 2f00bbe) * feat: add recording props to plugin (#2125) * fix(pwa): make cache filters more specifc * feat(plugins): add `record` prop to iframe plugins when caching dashboard * feat: remove cached plugin data with dashboard (cherry picked from commit 8cb24db) * chore: run Prettier (cherry picked from commit f50c4bb) * fix: pass correct prop for display property Also do not pass the whole userSettings object, not used in the plugins where the only thing really needed is the displayProperty for the analytics request. (cherry picked from commit a41f0ca) * fix: add more paths to omit from cache Replaced deprecated config key with new one. (cherry picked from commit d1fc48a) * fix: show error when iframe src is not available Also avoid adding postRobot handlers when the iframeRef is not available, this removes some extra errors. (cherry picked from commit 517fc06) * fix: do not pass filtered AO to LL plugin Filters don't work in LL without modifications. For now just disable filtering for LL items. (cherry picked from commit 8a06190) * fix: set error when pluginLaunchUrl is not available The app could be installed and the entry in apps api returned, but the plugin launch URL can be missing, in that case the plugin cannot be loaded in the iframe. (cherry picked from commit d6e703e) * refactor: use specific reducer selector (cherry picked from commit f952cfd) * fix: remove View As in context menu for LL items (cherry picked from commit b777a0d) * fix: clear selected from Redux when editing a new dashboard (cherry picked from commit 440f7c4) * feat: add tags on item header, used for filters in LL For LL items we don't apply filters. Show a tag in the item header to indicate this to the user. (cherry picked from commit 5683b14) * feat: add overlay on LL items to inform about not applied filters (cherry picked from commit 14d814e) * refactor: move the overlay handling to the parent component In this way both the tag in the item header and the overlay in the item content can be controlled at the same time allowing for toggling the tag depending on whether the overlay is shown or not. (cherry picked from commit dbd51ad) * fix: remove tag icon and fix styles Approved by Joe. (cherry picked from commit d15bae0) * fix: remove context menu when plugin is not installed * fetch schema for eventVisualizations, required by the interpretations component (cherry picked from commit 07bd0fc) * fix: rename Event visualizations to Line lists This appears as section title in the dashboard item search. (cherry picked from commit 14e2cea) * feat: add custom message for when the plugin is not installed More friendly than the generic error message. In this case there isn't any problem with the dashboard item, but the plugin for rendering the item is not installed. We inform the user about it and suggest to install the missing app from the app hub. (cherry picked from commit d9a7f0b) * refactor: propagate d2 needed to check installed apps (cherry picked from commit df797dd) * chore: update d2-ui-interpretations component (cherry picked from commit 520903f) * fix: fix re-render of visualizations when interpretations panel toggles (#2199) (cherry picked from commit 50a6b8d) * fix: detect plugin availability via plugin launch URL (cherry picked from commit 48b27e9) * chore: remove accidentally committed temp files (cherry picked from commit 4f79a95) * fix: use new TranslationDialog component (#2111) (cherry picked from commit ce58d7f) * fix: fix linting errors * test: fix failing tests * fix: add missing pieces to make LL plugin work * chore: generate pot file * chore: deduplicate yarn * fix: fix CSS class name used for error/warning messages * fix: pass the correct function to the Map plugin * fix: remove unused code * fix: use first of type logic also in edit and print mode * test: fix tests and snapshots * fix: use first of type logic also in print mode * fix: fix filtering for Maps dashboard items * fix: fix props passed to error component * fix: fix filter version prop handling * chore: deduplicate yarn.lock --------- Co-authored-by: Kai Vandivier <[email protected]>
janhenrikoverland
pushed a commit
that referenced
this pull request
Jun 2, 2023
* chore: convert to functional component (cherry picked from commit 4f0abd8) * feat: enable EVENT_VISUALIZATION dashboard item types (cherry picked from commit 1dd409c) * feat: allow iframe plugin overrides for dev/test (cherry picked from commit f79f622) * feat: render LL plugin in iframe (cherry picked from commit 29c5397) * chore: add dependency for post-robot * refactor: rewrite condition for better readability (cherry picked from commit 010be92) * fix: remove Suspense which might interfere with the iframe (cherry picked from commit d33336b) * refactor: use Layer + Popper to avoid bottom double border (cherry picked from commit cd0751d) * refactor: preserve height/width if provided, update props passed (cherry picked from commit e2fca17) * chore: remove unused import (cherry picked from commit 2f00bbe) * feat: add recording props to plugin (#2125) * fix(pwa): make cache filters more specifc * feat(plugins): add `record` prop to iframe plugins when caching dashboard * feat: remove cached plugin data with dashboard (cherry picked from commit 8cb24db) * fix: pass correct prop for display property Also do not pass the whole userSettings object, not used in the plugins where the only thing really needed is the displayProperty for the analytics request. (cherry picked from commit a41f0ca) * fix: add more paths to omit from cache Replaced deprecated config key with new one. (cherry picked from commit d1fc48a) * fix: show error when iframe src is not available Also avoid adding postRobot handlers when the iframeRef is not available, this removes some extra errors. (cherry picked from commit 517fc06) * fix: do not pass filtered AO to LL plugin Filters don't work in LL without modifications. For now just disable filtering for LL items. (cherry picked from commit 8a06190) * refactor: use specific reducer selector (cherry picked from commit f952cfd) * fix: remove View As in context menu for LL items (cherry picked from commit b777a0d) * fix: clear selected from Redux when editing a new dashboard (cherry picked from commit 440f7c4) * feat: add tags on item header, used for filters in LL For LL items we don't apply filters. Show a tag in the item header to indicate this to the user. (cherry picked from commit 5683b14) * feat: add overlay on LL items to inform about not applied filters (cherry picked from commit 14d814e) * refactor: move the overlay handling to the parent component In this way both the tag in the item header and the overlay in the item content can be controlled at the same time allowing for toggling the tag depending on whether the overlay is shown or not. (cherry picked from commit dbd51ad) * fix: remove tag icon and fix styles Approved by Joe. (cherry picked from commit d15bae0) * fix: fetch schema for eventVisualization, required by interpretations * fix: rename Event visualizations to Line lists This appears as section title in the dashboard item search. (cherry picked from commit 14e2cea) * feat: add custom message for when the plugin is not installed More friendly than the generic error message. In this case there isn't any problem with the dashboard item, but the plugin for rendering the item is not installed. We inform the user about it and suggest to install the missing app from the app hub. (cherry picked from commit d9a7f0b) * chore: update d2-ui-interpretations component (cherry picked from commit 520903f) * fix: fix re-render of visualizations when interpretations panel toggles (#2199) (cherry picked from commit 50a6b8d) * fix: add missing pieces to make LL plugin work * fix: fix CSS class name used for error/warning messages * fix: pass the correct function to the Map plugin * fix: remove unused code * fix: use first of type logic also in edit and print mode * fix: use first of type logic also in print mode * chore: run prettier * fix: use "old" components from @dhis2/ui to avoid bumping version * fix: lookup in installed apps for the plugin URL to use in the iframe (cherry picked from commit e733903) * fix: return LL plugin URL to use in iframe 2.38 api/apps does not return the pluginLaunchUrl. * fix: replaced Cover with ComponentCover * chore: deduplicate yarn.lock * chore: generate pot file * test: fix tests * test: add missing snapshot file * fix: fix filtering for Maps dashboard items * fix: fix props passed to error component * fix: fix filter version prop handling * fix: restored working yarn.lock * chore: run prettier * chore: deduplicate yarn.lock --------- Co-authored-by: Kai Vandivier <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The whole thing is quite complex, but basically the culprit is the
itemprop which changes itsh(height) attribute when the interpretations panel is toggled.This updated
itemobject is required by React grid.However, anything else in that object does not change when toggling the interpretations panel and re-render of the "visualizations" loaded in the item components is not necessary and not desired as it causes loading spinner and/or flashing of the visualization content.
The fix is basically to avoid relying on the whole
itemobject in components that should not re-render when the item's height change.Also, the
IframePluginis now wrapped in aReact.memoand that required memoizing all the values for the props passed to the plugin.Screenshots to show that the Visualization/IframePlugin components do not reload when the interpretations panel is toggled: