fix: calculate item height accounting for long title [DHIS2-8492] (#664)

fixes: https://jira.dhis2.org/browse/DHIS2-8492 Calculate the height of the item based on the header height - which is no longer constant since items now support wrapped titles. This calculation is the backup value incase the measured height of the chart is not available right away, as in view mode. In addition, fn getContentStyle was memoized to reduce chart rerenders when items are moved in edit mode.
dhis2 · Mar 20, 2020 · e1f73c9 · e1f73c9
1 parent 7fc9249
commit e1f73c9
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 27 deletions.
diff --git a/src/components/Item/ItemHeader.js b/src/components/Item/ItemHeader.js
@@ -8,19 +8,23 @@ import DeleteItemButton from './DeleteItemButton';
 
 import classes from './styles/ItemHeader.module.css';
 
+// This is the margin-top + margin-bottom defined in the css file
+export const HEADER_MARGIN_HEIGHT = 12;
+
 const ItemHeader = props => {
     const {
         title,
         editMode,
         actionButtons,
         itemId,
         acRemoveDashboardItem,
+        forwardedRef,
     } = props;
 
     const handleDeleteItem = () => acRemoveDashboardItem(itemId);
 
     return (
-        <div className={classes.itemHeaderWrap}>
+        <div className={classes.itemHeaderWrap} ref={forwardedRef}>
             <p className={classes.itemTitle}>{title}</p>
             {editMode ? (
                 <div className={classes.itemActionsWrap}>
@@ -41,10 +45,15 @@ ItemHeader.propTypes = {
     acRemoveDashboardItem: PropTypes.func,
     actionButtons: PropTypes.node,
     editMode: PropTypes.bool,
+    forwardedRef: PropTypes.object,
     itemId: PropTypes.string,
     title: PropTypes.string,
 };
 
+ItemHeader.defaultProps = {
+    forwardedRef: {},
+};
+
 const mapStateToProps = state => ({
     editMode: sGetIsEditing(state),
 });
@@ -53,7 +62,15 @@ const mapDispatchToProps = {
     acRemoveDashboardItem,
 };
 
-export default connect(
+const ConnectedItemHeader = connect(
     mapStateToProps,
     mapDispatchToProps
 )(ItemHeader);
+
+// TODO this is a false positive that is fixed in eslint-plugin-react v7.15
+// github.com/yannickcr/eslint-plugin-react/blob/master/CHANGELOG.md
+/* eslint-disable react/display-name */
+export default React.forwardRef((props, ref) => (
+    <ConnectedItemHeader {...props} forwardedRef={ref} />
+));
+/* eslint-enable react/display-name */
diff --git a/src/components/Item/VisualizationItem/Item.js b/src/components/Item/VisualizationItem/Item.js
@@ -8,7 +8,7 @@ import i18n from '@dhis2/d2-i18n';
 
 import DefaultPlugin from './DefaultPlugin';
 import FatalErrorBoundary from './FatalErrorBoundary';
-import ItemHeader from '../ItemHeader';
+import ItemHeader, { HEADER_MARGIN_HEIGHT } from '../ItemHeader';
 import ItemHeaderButtons from './ItemHeaderButtons';
 import ItemFooter from './ItemFooter';
 import * as pluginManager from './plugin';
@@ -30,8 +30,6 @@ import { colors } from '@dhis2/ui-core';
 import { getVisualizationConfig } from './plugin';
 import LoadingMask from './LoadingMask';
 
-const HEADER_HEIGHT = 45;
-
 const styles = {
     icon: {
         width: 16,
@@ -76,12 +74,15 @@ export class Item extends Component {
         this.d2 = context.d2;
 
         this.contentRef = React.createRef();
+        this.headerRef = React.createRef();
 
         this.memoizedApplyFilters = memoizeOne(this.applyFilters);
 
         this.memoizedGetVisualizationConfig = memoizeOne(
             getVisualizationConfig
         );
+
+        this.memoizedGetContentStyle = memoizeOne(this.getContentStyle);
     }
 
     async componentDidMount() {
@@ -171,7 +172,11 @@ export class Item extends Component {
         const props = {
             ...this.props,
             visualization,
-            style: this.getContentStyle(),
+            style: this.memoizedGetContentStyle(
+                this.props.item.originalHeight,
+                this.headerRef.current.clientHeight,
+                this.contentRef ? this.contentRef.offsetHeight : null
+            ),
         };
 
         switch (activeType) {
@@ -275,19 +280,13 @@ export class Item extends Component {
             this.props.visualization
         );
 
-    getContentStyle = () => {
-        const { item, editMode } = this.props;
-        const PADDING_BOTTOM = 4;
-
-        return !editMode
-            ? {
-                  height: item.originalHeight - HEADER_HEIGHT - PADDING_BOTTOM,
-              }
-            : {
-                  height: this.contentRef
-                      ? this.contentRef.offsetHeight
-                      : item.originalHeight - HEADER_HEIGHT - PADDING_BOTTOM,
-              };
+    getContentStyle = (originalHeight, headerHeight, measuredHeight) => {
+        const calculatedHeight =
+            originalHeight - headerHeight - HEADER_MARGIN_HEIGHT;
+
+        return {
+            height: measuredHeight || calculatedHeight,
+        };
     };
 
     render() {
@@ -312,6 +311,7 @@ export class Item extends Component {
                     title={pluginManager.getName(item)}
                     itemId={item.id}
                     actionButtons={actionButtons}
+                    ref={this.headerRef}
                 />
                 <FatalErrorBoundary>
                     <div

diff --git a/src/components/Item/VisualizationItem/__tests__/Item.spec.js b/src/components/Item/VisualizationItem/__tests__/Item.spec.js
@@ -23,6 +23,8 @@ jest.mock('../plugin', () => {
     };
 });
 
+const mockHeaderRef = { clientHeight: 50 };
+
 describe('VisualizationItem/Item', () => {
     let props;
     let shallowItem;
@@ -75,6 +77,8 @@ describe('VisualizationItem/Item', () => {
 
         const component = canvas();
 
+        component.instance().headerRef.current = mockHeaderRef;
+
         component.setState({ configLoaded: true });
 
         const visPlugin = component.find(VisualizationPlugin);
@@ -101,6 +105,7 @@ describe('VisualizationItem/Item', () => {
         };
 
         const component = canvas();
+        component.instance().headerRef.current = mockHeaderRef;
 
         component.setState({ configLoaded: true });
 
@@ -129,6 +134,7 @@ describe('VisualizationItem/Item', () => {
         expect(canvas()).toMatchSnapshot();
 
         const component = canvas();
+        component.instance().headerRef.current = mockHeaderRef;
 
         component.setState({ configLoaded: true });
 

diff --git a/src/components/Item/VisualizationItem/__tests__/__snapshots__/Item.spec.js.snap b/src/components/Item/VisualizationItem/__tests__/__snapshots__/Item.spec.js.snap
@@ -53,7 +53,7 @@ ShallowWrapper {
     "nodeType": "function",
     "props": Object {
       "children": Array [
-        <Connect(ItemHeader)
+        <ForwardRef
           actionButtons={
             <ItemHeaderButtons
               activeFooter={false}
@@ -97,7 +97,7 @@ ShallowWrapper {
       Object {
         "instance": null,
         "key": undefined,
-        "nodeType": "class",
+        "nodeType": "function",
         "props": Object {
           "actionButtons": <ItemHeaderButtons
             activeFooter={false}
@@ -128,9 +128,14 @@ ShallowWrapper {
           "itemId": undefined,
           "title": "rainbow",
         },
-        "ref": null,
+        "ref": Object {
+          "current": null,
+        },
         "rendered": null,
-        "type": [Function],
+        "type": Object {
+          "$$typeof": Symbol(react.forward_ref),
+          "render": [Function],
+        },
       },
       Object {
         "instance": null,
@@ -167,7 +172,7 @@ ShallowWrapper {
       "nodeType": "function",
       "props": Object {
         "children": Array [
-          <Connect(ItemHeader)
+          <ForwardRef
             actionButtons={
               <ItemHeaderButtons
                 activeFooter={false}
@@ -211,7 +216,7 @@ ShallowWrapper {
         Object {
           "instance": null,
           "key": undefined,
-          "nodeType": "class",
+          "nodeType": "function",
           "props": Object {
             "actionButtons": <ItemHeaderButtons
               activeFooter={false}
@@ -242,9 +247,14 @@ ShallowWrapper {
             "itemId": undefined,
             "title": "rainbow",
           },
-          "ref": null,
+          "ref": Object {
+            "current": null,
+          },
           "rendered": null,
-          "type": [Function],
+          "type": Object {
+            "$$typeof": Symbol(react.forward_ref),
+            "render": [Function],
+          },
         },
         Object {
           "instance": null,